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