[PATCH] D14325: [clang-format] Do not align assignments that aren't after the same number of commas. (Closes: 25329)

[Daniel Jasper via cfe-commits]

djasper added a comment.

Sorry for being so slow to respond. Feel free to ping me more frequently ;-).


================
Comment at: lib/Format/WhitespaceManager.cpp:197
@@ +196,3 @@
+  // Keep track of the scope depth of matching tokens. We will only align a
+  // sequence of matching token that share the same scope depth.
+  unsigned DepthOfPrevToken = 0;
----------------
Maybe be slightly more specific about what constitutes a "scope".

Also, FormatTokens already have this information stored in NestingLevel. Would it maybe make sense to copy that information into each Change? In the long run, we might actually wanna switch to storing a back-reference to the actual token in a change to have access to all the information. But that would probably better be done in a subsequent change.

================
Comment at: lib/Format/WhitespaceManager.cpp:208
@@ +207,3 @@
+  // Whether a matching token has been found on the current line.
+  bool FoundOnLine = false;
+
----------------
FoundOnLine seems to be missing some crucial information. Would you might changing to FoundMathcingTokenOnLine or FoundMatchOnLine?

================
Comment at: lib/Format/WhitespaceManager.cpp:239
@@ -321,20 +238,3 @@
 
-    if (Changes[i].Kind == tok::r_brace) {
-      if (!FoundLeftBraceOnLine)
-        AlignSequence();
-      FoundLeftBraceOnLine = false;
-    } else if (Changes[i].Kind == tok::l_brace) {
-      FoundLeftBraceOnLine = true;
-      if (!FoundDeclarationOnLine)
-        AlignSequence();
-    } else if (Changes[i].Kind == tok::r_paren) {
-      if (!FoundLeftParenOnLine)
-        AlignSequence();
-      FoundLeftParenOnLine = false;
-    } else if (Changes[i].Kind == tok::l_paren) {
-      FoundLeftParenOnLine = true;
-      if (!FoundDeclarationOnLine)
-        AlignSequence();
-    } else if (!FoundDeclarationOnLine && !FoundLeftBraceOnLine &&
-               !FoundLeftParenOnLine && Changes[i].IsStartOfDeclName) {
-      FoundDeclarationOnLine = true;
+    if (Changes[i].Kind == tok::comma) {
+      ++CommasBeforeToken;
----------------
I think this would be slightly easier to read if you used "continue" (basically instead of each "else" here)
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

================
Comment at: unittests/Format/FormatTest.cpp:8662
@@ -8661,3 +8661,3 @@
                "public:\n"
-               "  int i = 1;\n"
+               "  int i            = 1;\n"
                "  virtual void f() = 0;\n"
----------------
I am not sure that this is actually desirable, but then again, I also don't think it matters. If people don't like it, they can add a blank line or a comment in-between.


http://reviews.llvm.org/D14325