[PATCH] clang-format: Add ability to align assignment operators

Daniel Jasper djasper at google.com
Wed Apr 29 03:21:29 PDT 2015


I think this basically looks good. Don't worry about the test changes, they all look trivial.

Btw., do you have commit access?


================
Comment at: lib/Format/WhitespaceManager.cpp:158
@@ +157,3 @@
+  auto AlignSequence =
+      [this, &MinColumn, &StartOfSequence, &EndOfSequence]() mutable {
+    alignConsecutiveAssignments(StartOfSequence, EndOfSequence, MinColumn);
----------------
I think just using:

  auto AlignSequence = [&] { ... };

is fine for this locally-scoped lambda. But I am not an expert using lambdas. AFAIU, at least the "mutable" should be unnecessary as it only influences parameters captured by copy and all of yours are by reference, I think.

================
Comment at: lib/Format/WhitespaceManager.cpp:165
@@ +164,3 @@
+
+  for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
+    if (Changes[i].NewlinesBefore != 0) {
----------------
I was thinking more of very high-level comments that help understand the code, e.g.:

  // Walk through all of the changes and find sequences of "=" to align.
  // To do so, keep track of the lines and whether or not an "=" was found on
  // align. If a "=" is found on a line, extend the current sequence. If the current
  // line cannot be part of a sequence, e.g. because there is an empty line
  // before it or it contains non-assignments, finalize the previous sequence.

================
Comment at: lib/Format/WhitespaceManager.cpp:180-184
@@ +179,7 @@
+
+    // If the current token is an equals and we found an assignment on this
+    // line already or there was a newline in this or the next change: check
+    // if we have a started sequence and align it. Or if we haven't found a
+    // left parenthesis on this line yet, but we just found a right one:
+    // check for and align the sequence.
+    if ((Changes[i].Kind == tok::equal &&
----------------
For me personally, this is too detailed and too much of a replication of the source code in words. But this is probably a matter of taste.

================
Comment at: lib/Format/WhitespaceManager.cpp:200
@@ +199,3 @@
+      // line yet, but we just found an equals token:rRemember we've now found
+      // an
+      // assignment, change the end of the sequence. Then calculate the new min
----------------
reflow.

================
Comment at: lib/Format/WhitespaceManager.cpp:215
@@ +214,3 @@
+
+  // Align the last sequence if there was one
+  if (StartOfSequence > 0) {
----------------
nit: append a period.

http://reviews.llvm.org/D8821

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list