[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 inline comments.

================
Comment at: lib/Format/WhitespaceManager.cpp:197
@@ +196,3 @@
+  // Keep track of the nesting level of matching tokens, i.e. the number of
+  // surrounding (), [], or {}. We will only align a sequence of matching
+  // token that share the same scope depth.
----------------
berenm wrote:
> I have added some details in the comments, but couldn't manage to use the FormatToken::NestingLevel member. I haven't figured out why exactly, but it doesn't increase the nesting level counter in the braced scope of examples like:
> 
> ```
> int l = []() {
>   int i = 0;
>   int h = 0;
> }
> ```
> 
> i.e.: tokens `i` and `h` have `NestingLevel == 0` as it is the case for token `l`.
> 
> I will try to figure out why exactly but it will require a bit more time.
Ah, right. It's fine to do this later. The reason is that the lines in the nested block are handled separately and scope counting starts from the beginning. You could probably use the line level somehow, but that's probably insufficient as it doesn't count the scopes in the outer statement.

================
Comment at: lib/Format/WhitespaceManager.cpp:263
@@ -321,20 +262,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 (Matches(Changes[i])) {
+      // If there is more than one matching token per line, or if the number of
----------------
berenm wrote:
> I did that.
> 
> It makes me think that the code actually forbids the generic `AlignToken` function to be used with a matcher for aligning commas or braces, paren and brackets...
Use:

  if (!Matches(Changes[i]))
    continue;

================
Comment at: lib/Format/WhitespaceManager.cpp:263-274
@@ -321,21 +262,14 @@
 
-    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 (Matches(Changes[i])) {
+      // If there is more than one matching token per line, or if the number of
+      // preceding commas, or the scope depth, do not match anymore, end the
+      // sequence.
+      if (FoundMatchOnLine || CommasBeforeMatch != CommasBeforeLastMatch ||
+          NestingLevel != NestingLevelOfLastMatch)
+        AlignCurrentSequence();
+
+      CommasBeforeLastMatch = CommasBeforeMatch;
+      NestingLevelOfLastMatch = NestingLevel;
+      FoundMatchOnLine = true;
+
       if (StartOfSequence == 0)
----------------
djasper wrote:
> berenm wrote:
> > I did that.
> > 
> > It makes me think that the code actually forbids the generic `AlignToken` function to be used with a matcher for aligning commas or braces, paren and brackets...
> Use:
> 
>   if (!Matches(Changes[i]))
>     continue;
I don't understand what you mean.


http://reviews.llvm.org/D14325