This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix alignConsecutiveAssignments not working properly
ClosedPublic

Authored by berenm on Aug 26 2015, 9:21 AM.

Details

Reviewers
djasper
matto1990
Summary

This simple test case (added to the unit tests) was failing to align, apparently due to the method(); on the last line:

int oneTwoThree = 123;
int oneTwo      = 12;
method();

The unit tests are all passing, including this new one.

Diff Detail

Event Timeline

berenm updated this revision to Diff 33210.Aug 26 2015, 9:21 AM
berenm retitled this revision from to [clang-format] Fix alignConsecutiveAssignments not working properly.
berenm updated this object.
berenm added a reviewer: djasper.
berenm added a subscriber: cfe-commits.
berenm updated this object.Aug 26 2015, 9:21 AM

I'm not entirely sure of what was going on, so I rewrote the code a bit, and hopefully, more clearly.

lib/Format/WhitespaceManager.cpp
218

What was the purpose of PreviousShift?

229

Why shifting by Shift and then also by PreviousShift?

berenm updated this object.Aug 26 2015, 9:24 AM
djasper edited edge metadata.

Adding Matt, the original author of the code. (also I don't have much time to review this or the other patch today, but will try to finish before the end of the week)

matto1990 requested changes to this revision.Sep 2 2015, 12:30 AM
matto1990 edited edge metadata.

This looks much better than the code I wrote. If all the unit tests are passing, the one change I can see to make is a misspelling in one of the comments. The rest looks excellent though!

lib/Format/WhitespaceManager.cpp
171

Nit: We instead of Wee

218

If I remember correctly it was to measure the maximum shift needed on the lines previous. The name Shift makes more sense actually.

229

I don't remember entirely. I think though that in the cases where both are applied the value will be the same. Either way, what you have now looks much cleaner.

This revision now requires changes to proceed.Sep 2 2015, 12:30 AM
berenm updated this revision to Diff 33783.Sep 2 2015, 1:21 AM
berenm edited edge metadata.

Here it is, with the typos in comments corrected, and rebased over the latest trunk.

Thanks for the review!

berenm marked 5 inline comments as done.Sep 2 2015, 1:21 AM
matto1990 accepted this revision.Sep 8 2015, 4:07 AM
matto1990 edited edge metadata.

Looks good to me now!

This revision is now accepted and ready to land.Sep 8 2015, 4:07 AM

I don't have credentials to commit it, anybody could do it for me if the diff is ok?

Thanks!

djasper closed this revision.Sep 22 2015, 2:33 AM

Submitted as r248254, thank you!