[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Manuel Klimek via Phabricator via cfe-commits]

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