[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

[Daniel Jasper via cfe-commits]

djasper added a comment.

So sorry. Seems I forgot to hit "Submit" :(.

If you don't like the ".first" and ".second" of the pair, you could introduce a struct for it and overload operator<. Might actually be more readable.



> WhitespaceManager.cpp:73
> +      Tok.NestingLevel,
>        /*Spaces=*/0, Tok.OriginalColumn, Tok.NewlinesBefore, "", "",
> +      Tok.Tok.getKind(), Tok.Type, InPPDirective && !Tok.IsFirst,

nit: Move these to the previous line. clang-format won't do that, because of the comment, but that's actually irrelevant here.

> WhitespaceManager.cpp:183
> +
> +    // NestingLevel is raised on the opening paren/square, and remains raised
> +    // until AFTER the closing paren/square. This means that with a statement

I don't see why this would be necessary. If I remove it, all tests do still pass.

> WhitespaceManager.cpp:190
> +    //
> +    // The "int x = 1" line is going to have the same NestingLevel as
> +    // the tokens inside the parentheses of the "for" statement.

Also, this comment seems wrong? The "int x = 1;" actually starts a new (child) line. If that has the same nesting level, that seems like a bug we need to fix.

> WhitespaceManager.cpp:210
> +  // We only run the "Matches" function on tokens from the outer-most scope.
> +  // However, we do need to adjust some special tokens within the entire
> +  // Start..End range, regardless of their scope. This special rule applies

Make this (and maybe a few others) more concrete. Don't write "some special tokens", write what they actually are.

> WhitespaceManager.cpp:214
> +  // who's function names have been shifted for declaration alignment's sake.
> +  // See "split function parameter alignment" in FormatTest.cpp for an example.
> +  SmallVector<TokenTypeAndLevel, 16> ScopeStack;

If the example isn't too long, writing the source code in the comment seems better than referencing the test.

> WhitespaceManager.cpp:389
>  
> -  EndOfSequence = Changes.size();
> +  unsigned StoppedAt = i;
> +  EndOfSequence = i;

Either do:

  EndOfSequence = StoppedAt;

or just remove StoppedAt and use i.

> FormatTest.cpp:9364
> +  // WhitespaceManager.cpp
> +  EXPECT_EQ("double a(int x);\n"
> +            "int    b(int    x,\n"

Can you add a test case where there is a line wrap after the "("?

https://reviews.llvm.org/D21279