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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
