klimek added inline comments.
================
Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+ F &&Matches,
----------------
VelocityRa wrote:
> 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.
How about calling this AlignMacroSequence and the other one AlignMacros.
Comment here would be something like (as I don't think "scope" plays a role??)
// Aligns a sequence of macro definitions.
The problem is that I think the alignTokensAll don't make sense either as a
function.
We should be able to inline this into alignConsecutiveMacros, and pull the
std::function handed in either into its own function, or just define it as the
first step in alignConsecutiveMacros.
================
Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+ F &&Matches,
+ SmallVector<WhitespaceManager::Change, 16> &Changes) {
----------------
VelocityRa wrote:
> 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.
I think we need to look at this from first principles. We can fix the old code
later if it doesn't make sense :)
================
Comment at: lib/Format/WhitespaceManager.cpp:442
+
+ for (unsigned i = Start; i != End; ++i) {
+ if (Changes[i].NewlinesBefore > 0) {
----------------
VelocityRa wrote:
> klimek wrote:
> > llvm style: use an upper case I for index vars.
> Ok, I assume your style changed because this is copied from
> `AlignTokenSequence`.
Yea, sorry, those are in violation (we should fix that at some point, but *not*
in this patch :)
================
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
----------------
VelocityRa wrote:
> 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.
I was suggesting to move it inside the if below, did you try that (sounds like
you tried to remove it).
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
