[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