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

[Nick Renieris via Phabricator via llvm-commits]

VelocityRa marked 11 inline comments as done.
VelocityRa added a comment.

Waiting for further feedback before pushing an update.



================
Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+                      F &&Matches,
----------------
klimek wrote:
> I don't think the 'All' postfix in the name is helpful. What are you trying to say with that name?
I'm not particularly fond of `All` either, suggestions welcome.

As the comment above explains, `All` refers to the fact that it operates on all tokens, instead of being limited to certain cases like `AlignTokenSequence` is. Maybe I should name //this// one `AlignTokenSequence` and the other one `AlignTokenSequenceOuterScope`, or something.


================
Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+                      F &&Matches,
+                      SmallVector<WhitespaceManager::Change, 16> &Changes) {
----------------
klimek wrote:
> Why an rvalue ref? Also, this is used only once, so why make this a template?
It's an rvalue ref, because that's also the case for `AlignTokenSequence` (wasn't sure why, so I left it as is).
It's used only once, but the function is more generic that way, no? That's the point of its generic name.
Tell me if I should change it.


================
Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+    if (Changes[i].NewlinesBefore > 0) {
----------------
klimek wrote:
> llvm style: use an upper case I for index vars.
Ok, I assume your style changed because this is copied from `AlignTokenSequence`.


================
Comment at: lib/Format/WhitespaceManager.cpp:473
+
+  // Line number of the start and the end of the current token sequence.
+  unsigned StartOfSequence = 0;
----------------
klimek wrote:
> These are indices into Matches, not line numbers, right?
Correct. My bad.


================
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
----------------
klimek wrote:
> Why set EndOfSequence outside the if below?
It's from `AlignTokens`. I think it's because due to some of the loop logic, it ends up not checking up to the point of the the last token.
Without setting this and calling `AlignCurrentSequence()` once more at the end, the last line of a macro group does not get properly aligned, the tests fail.


================
Comment at: lib/Format/WhitespaceManager.cpp:512
+
+    // If there is more than one matching token per line, end the sequence.
+    if (FoundMatchOnLine)
----------------
klimek wrote:
> What's the reason for this?
I don't remember, but it was unnecessary apparently. The tests pass without this check.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D28462/new/

https://reviews.llvm.org/D28462