[PATCH] D12369: [clang-format] Fix alignConsecutiveAssignments not working properly

[Matt Oakes via cfe-commits]

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