matto1990 requested changes to this revision.
matto1990 added a comment.
This revision now requires changes to proceed.
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!
================
Comment at: lib/Format/WhitespaceManager.cpp:171
@@ +170,3 @@
+ //
+ // Wee need to adjust the StartOfTokenColumn of each Change that is on a line
+ // containing any assignment to be aligned and located after such assignment
----------------
Nit: //We// instead of //Wee//
================
Comment at: lib/Format/WhitespaceManager.cpp:218
@@ -217,3 +229,1 @@
- bool AlignedAssignment = false;
- int PreviousShift = 0;
for (unsigned i = Start; i != End; ++i) {
----------------
berenm wrote:
> What was the purpose of `PreviousShift`?
If I remember correctly it was to measure the maximum shift needed on the lines
previous. The name `Shift` makes more sense actually.
================
Comment at: lib/Format/WhitespaceManager.cpp:229
@@ -228,3 +244,3 @@
assert(Shift >= 0);
- Changes[i].Spaces += Shift;
if (i + 1 != Changes.size())
----------------
berenm wrote:
> Why shifting by `Shift` //and then// also by `PreviousShift`?
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.
http://reviews.llvm.org/D12369
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
