klimek added a comment.
In D28462#1406716 <https://reviews.llvm.org/D28462#1406716>, @jacob-keller
wrote:
> In regards to whether clang-format should support this feature, I think it's
> pretty clear that a large number of users want it to. The tool already
> supports formatting various other definitions in a similar manner, including
> variables and struct members. So I don't see how a similar features which
> aligns consecutive macros is something which doesn't make sense.
>
> Additionally, because the formatting will *undo* such formatting if done
> manually, it means that the existing source code formatting this way cannot
> be handled via clang-format. In my case, it essentially means that I cannot
> use clang-format to enforce the style guidelines, as the macros definitions
> are aligned.
>
> Additionally, aligning a series of macros makes sense because it helps
> improve readability, and is thus something that I'd rather not lose if I were
> to switch to using clang-format without the feature.
================
Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+ F &&Matches,
----------------
I don't think the 'All' postfix in the name is helpful. What are you trying to
say with that name?
================
Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+ F &&Matches,
+ SmallVector<WhitespaceManager::Change, 16> &Changes) {
----------------
Why an rvalue ref? Also, this is used only once, so why make this a template?
================
Comment at: lib/Format/WhitespaceManager.cpp:442
+
+ for (unsigned i = Start; i != End; ++i) {
+ if (Changes[i].NewlinesBefore > 0) {
----------------
llvm style: use an upper case I for index vars.
================
Comment at: lib/Format/WhitespaceManager.cpp:473
+
+ // Line number of the start and the end of the current token sequence.
+ unsigned StartOfSequence = 0;
----------------
These are indices into Matches, not line numbers, right?
================
Comment at: lib/Format/WhitespaceManager.cpp:497
+
+ unsigned i = 0;
+ for (unsigned e = Changes.size(); i != e; ++i) {
----------------
llvm style: use upper-case I and E.
================
Comment at: lib/Format/WhitespaceManager.cpp:500
+ if (Changes[i].NewlinesBefore != 0) {
+ EndOfSequence = i;
+ // If there is a blank line, or if the last line didn't contain any
----------------
Why set EndOfSequence outside the if below?
================
Comment at: lib/Format/WhitespaceManager.cpp:512
+
+ // If there is more than one matching token per line, end the sequence.
+ if (FoundMatchOnLine)
----------------
What's the reason for this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D28462/new/
https://reviews.llvm.org/D28462
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
