Page MenuHomePhabricator

[clang-format] Do not align assignments that aren't after the same number of commas. (Closes: 25329)
ClosedPublic

Authored by berenm on Nov 4 2015, 2:18 AM.

Details

Reviewers
djasper
Summary

This fixes https://llvm.org/bugs/show_bug.cgi?id=25329, as well as misalignments like the following:

int a, b = 2;
int c    = 3;

Diff Detail

Event Timeline

berenm updated this revision to Diff 39173.Nov 4 2015, 2:18 AM
berenm retitled this revision from to [clang-format] Do not align assignments that aren't after the same number of commas. (Closes: 25329).
berenm updated this object.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
berenm updated this object.Nov 4 2015, 2:19 AM
berenm updated this revision to Diff 39201.Nov 4 2015, 6:59 AM

[clang-format] Count the number of braces and parens on the line instead of remembering only one.

berenm added a comment.Nov 4 2015, 7:00 AM

I've also added a fix for the other issue reported on the same bug. I could split the two reviews if necessary.

djasper added inline comments.Nov 4 2015, 7:08 AM
lib/Format/WhitespaceManager.cpp
150–151

Hm. So many lokal variables. I think it might make sense to wrap these in a class/struct? That could be a first step to re-using code in alignConsecutiveDeclarations.

Also, a comment would help here. Specifically, a comment explaining why you count commas, braces and parentheses (not explaining that this variables count them ;-) )

151

Use (here and everywhere else):

--LeftBracesOnLine;
berenm updated this revision to Diff 39232.Nov 4 2015, 11:35 AM

[clang-format] Alignment code factorization.

berenm updated this revision to Diff 39233.Nov 4 2015, 11:38 AM

[clang-format] Remove deleted methods declaration.

djasper edited edge metadata.Nov 4 2015, 3:07 PM

I like it :-)

lib/Format/WhitespaceManager.cpp
150–151

Why is this a class/struct? It doesn't seem to have any state.

berenm updated this revision to Diff 39269.Nov 4 2015, 3:15 PM
berenm marked 2 inline comments as done.
berenm edited edge metadata.

[clang-format] Replace TokenSequenceAligner with simple static functions.

berenm marked an inline comment as done.Nov 4 2015, 3:16 PM
berenm added inline comments.
lib/Format/WhitespaceManager.cpp
150–156

Haha, good question. I initially moved the variable there, but then it didn't looked really better, so I moved the variables back into the function...

berenm marked an inline comment as done.Nov 18 2015, 10:54 AM

Ping?

berenm updated this revision to Diff 41271.Nov 26 2015, 1:24 PM

[clang-format] alignConsecutiveXXX: replace the buggy paren / braces counter with a better scope tracker.

Updated the diff with a fix for the issue reported in http://lists.llvm.org/pipermail/cfe-dev/2015-November/046057.html

Sorry for being so slow to respond. Feel free to ping me more frequently ;-).

lib/Format/WhitespaceManager.cpp
197

Maybe be slightly more specific about what constitutes a "scope".

Also, FormatTokens already have this information stored in NestingLevel. Would it maybe make sense to copy that information into each Change? In the long run, we might actually wanna switch to storing a back-reference to the actual token in a change to have access to all the information. But that would probably better be done in a subsequent change.

208

FoundOnLine seems to be missing some crucial information. Would you might changing to FoundMathcingTokenOnLine or FoundMatchOnLine?

242–243

I think this would be slightly easier to read if you used "continue" (basically instead of each "else" here)
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

unittests/Format/FormatTest.cpp
8662

I am not sure that this is actually desirable, but then again, I also don't think it matters. If people don't like it, they can add a blank line or a comment in-between.

berenm updated this revision to Diff 41296.Nov 27 2015, 5:31 AM

[clang-format] Code cleanup and variable naming in WhitespaceManager::AlignTokens

berenm marked 2 inline comments as done.Nov 27 2015, 5:38 AM
berenm added inline comments.
lib/Format/WhitespaceManager.cpp
197

I have added some details in the comments, but couldn't manage to use the FormatToken::NestingLevel member. I haven't figured out why exactly, but it doesn't increase the nesting level counter in the braced scope of examples like:

int l = []() {
  int i = 0;
  int h = 0;
}

i.e.: tokens i and h have NestingLevel == 0 as it is the case for token l.

I will try to figure out why exactly but it will require a bit more time.

242–243

I did that.

It makes me think that the code actually forbids the generic AlignToken function to be used with a matcher for aligning commas or braces, paren and brackets...

djasper added inline comments.Nov 27 2015, 5:44 AM
lib/Format/WhitespaceManager.cpp
197

Ah, right. It's fine to do this later. The reason is that the lines in the nested block are handled separately and scope counting starts from the beginning. You could probably use the line level somehow, but that's probably insufficient as it doesn't count the scopes in the outer statement.

242–243

Use:

if (!Matches(Changes[i]))
  continue;
242–243

I don't understand what you mean.

berenm updated this revision to Diff 41299.Nov 27 2015, 5:50 AM
berenm marked an inline comment as done.

Fix continue statements.

berenm added inline comments.Nov 27 2015, 5:53 AM
lib/Format/WhitespaceManager.cpp
242–243

The code was only checking for a match if none of the previous conditions were true. I changed that and with your suggestion so I guess it's OK now.

djasper accepted this revision.Nov 27 2015, 5:57 AM
djasper edited edge metadata.

Looks awesome :-)

lib/Format/WhitespaceManager.cpp
242–243

Ah, I see, I guess the initial if-else chains were too complicated for me ;-).

This looks good now, thanks.

This revision is now accepted and ready to land.Nov 27 2015, 5:57 AM

I don't have commit access, if you don't mind landing it for me :)

djasper closed this revision.Dec 1 2015, 4:05 AM

Submitted as r254406.

Note that I had to make the struct "Change" public. How did this compile on your end?

berenm added a comment.Dec 1 2015, 5:46 AM

I don't have any compilation error with GCC 5.2.1. I tried again with Clang 3.8 and indeed there is this error:

tools/clang/lib/Format/WhitespaceManager.cpp:155:51: error: 'Change' is a private member of 'clang::format::WhitespaceManager'
                   SmallVector<WhitespaceManager::Change, 16> &Changes) {
                                                  ^
tools/clang/lib/Format/WhitespaceManager.h:87:10: note: declared private here
  struct Change {
         ^

This can easily be reproduced: http://goo.gl/CUk665 vs http://goo.gl/6KcBFT

I don't know precisely which one is correct here, it looks like to be related to the access permission in the context of a template function instantiation.