[clang-format] Fix line lengths w/ comments in align

[JakeMerdichAMD]

Summary

https://bugs.llvm.org/show_bug.cgi?id=43845

When a '//comment' trails a consecutive alignment, it adds a whitespace
replacement within the comment token. This wasn't handled correctly in
the alignment code, which treats it as a whole token and thus double
counts it.

This can wrongly trigger the "line too long, it'll wrap" alignment-break
condition with specific lengths, causing the alignment to break for
seemingly no reason.

[JakeMerdichAMD]

The failing case in this commit looks like the following after formatting (with alignconsecutiveassignments and a specific column limit)

int x = 0;
int yy = 1; ///specificlennospace
int zzz = 2;

See PR43845 for more failing cases besides this.

[MyDeveloperDay]

I don't seem to be able to reproduce the original problem (either will your test case or the bad.h)

It would be good if the PR had the exact .clang-format file being used.

[JakeMerdichAMD]

Mind looking again? They did add one later on I think. It helped a lot since the exact settings and whitespace to trigger this are really finicky (though reproducible). You're right that it won't reproduce without it.

Here's a run log of the failing test I added, with the functional change commented out so it actually fails. Does this reproduce on your machine?

jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ git log --oneline -n2
9b146e8534b (HEAD -> pr43845) Fix line lengths w/ comments in align
d05f8a38c54 (origin/master, origin/HEAD) [ARM] VMOVrh of VMOVhr
jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ git diff
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index 5f6bde9f2d4..b2c6e6a16a2 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -412,7 +412,7 @@ static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
     int LineLengthAfter = -Changes[i].Spaces;
     for (unsigned j = i; j != e && Changes[j].NewlinesBefore == 0; ++j) {
       LineLengthAfter += Changes[j].Spaces;
-      if (!Changes[j].IsInsideToken)
+      //if (!Changes[j].IsInsideToken)
         LineLengthAfter += Changes[j].TokenLength;
     }
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ ninja -j12 FormatTests
ninja: no work to do.
jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$ tools/clang/unittests/Format/FormatTests --gtest_filter="*AlignConsecutiveAss*"
Note: Google Test filter = *AlignConsecutiveAss*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.AlignConsecutiveAssignments
/opt/ws/llvm-project/clang/unittests/Format/FormatTest.cpp:11576: Failure
      Expected: "int x   = 0;\n" "int yy  = 1; /// specificlennospace\n" "int zzz = 2;\n"
      Which is: "int x   = 0;\nint yy  = 1; /// specificlennospace\nint zzz = 2;\n"
To be equal to: format("int x   = 0;\n" "int yy  = 1; ///specificlennospace\n" "int zzz = 2;\n", Alignment)
      Which is: "int x = 0;\nint yy = 1; /// specificlennospace\nint zzz = 2;\n"
With diff:
@@ -1,3 +1,3 @@
-int x   = 0;
-int yy  = 1; /// specificlennospace
+int x = 0;
+int yy = 1; /// specificlennospace
 int zzz = 2;\n

[  FAILED  ] FormatTest.AlignConsecutiveAssignments (126 ms)
[----------] 1 test from FormatTest (126 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (127 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FormatTest.AlignConsecutiveAssignments

 1 FAILED TEST
jmerdich@JM-LDEV1:/opt/ws/llvm-project/build$

[MyDeveloperDay]

I've been able to reproduce and the patch looks good, I've just not had a chance to read the review in-depth (I don't know this section of the code as well as other parts).

[MyDeveloperDay]

clang/lib/Format/WhitespaceManager.cpp

452

could you help us here with a comment, I don't understand what they mean by Change.IsInsideToken?

[JakeMerdichAMD]

Add a comment explaining why checking IsInsideToken is needed here.

[JakeMerdichAMD]

Rebase to fix merge conflict

[MyDeveloperDay]

LGTM

[JakeMerdichAMD]

Hey @MyDeveloperDay, can I get your assistance committing this when you have the chance?

clang/lib/Format/WhitespaceManager.cpp

for (unsigned e = Changes.size(); i != e; ++i) {
     CommasBeforeLastMatch = CommasBeforeMatch;
     FoundMatchOnLine = true;
 
     if (StartOfSequence == 0)
       StartOfSequence = i;
 
     unsigned ChangeMinColumn = Changes[i].StartOfTokenColumn;
     int LineLengthAfter = Changes[i].TokenLength;
-    for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j)
-      LineLengthAfter += Changes[j].Spaces + Changes[j].TokenLength;
+    for (unsigned j = i + 1; j != e && Changes[j].NewlinesBefore == 0; ++j) {
+      LineLengthAfter += Changes[j].Spaces;
+      // Changes are generally 1:1 with the tokens, but a change could also be
+      // inside of a token, in which case it's counted more than once: once for
+      // the whitespace surrounding the token (!IsInsideToken) and once for
+      // each whitespace change within it (IsInsideToken).
+      // Therefore, changes inside of a token should only count the space.
+      if (!Changes[j].IsInsideToken)
+        LineLengthAfter += Changes[j].TokenLength;
+    }
     unsigned ChangeMaxColumn = Style.ColumnLimit - LineLengthAfter;
 
     // If we are restricted by the maximum column width, end the sequence.
     if (ChangeMinColumn > MaxColumn || ChangeMaxColumn < MinColumn ||
         CommasBeforeLastMatch != CommasBeforeMatch) {
       AlignCurrentSequence();
       StartOfSequence = i;
     }

clang/unittests/Format/FormatTest.cpp

verifyFormat("int aa       = ((1 > 2) ? 3 : 4);\n"
                Alignment);
   verifyFormat("for (int i = 0; i < 1; i++)\n"
                "  int x = 1;\n",
                Alignment);
   verifyFormat("for (i = 0; i < 1; i++)\n"
                "  x = 1;\n"
                "y = 1;\n",
                Alignment);
+
+  Alignment.ReflowComments = true;
+  Alignment.ColumnLimit = 50;
+  EXPECT_EQ("int x   = 0;\n"
+            "int yy  = 1; /// specificlennospace\n"
+            "int zzz = 2;\n",
+            format("int x   = 0;\n"
+                   "int yy  = 1; ///specificlennospace\n"
+                   "int zzz = 2;\n",
+                   Alignment));
 }
 
 TEST_F(FormatTest, AlignConsecutiveDeclarations) {
   FormatStyle Alignment = getLLVMStyle();
   Alignment.AlignConsecutiveMacros = true;
   Alignment.AlignConsecutiveDeclarations = false;
   verifyFormat("float const a = 5;\n"
                "int oneTwoThree = 123;",