Page MenuHomePhabricator

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

Authored by JakeMerdichAMD on May 5 2020, 6:30 PM.

Details

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.

Diff Detail

Event Timeline

JakeMerdichAMD created this revision.May 5 2020, 6:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 6:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
JakeMerdichAMD added a project: Restricted Project.May 5 2020, 6:36 PM

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.

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.

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 edited the summary of this revision. (Show Details)May 6 2020, 8:08 AM
MyDeveloperDay retitled this revision from Fix line lengths w/ comments in align to [clang-format] Fix line lengths w/ comments in align.

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 added inline comments.May 14 2020, 9:04 AM
clang/lib/Format/WhitespaceManager.cpp
452

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

Add a comment explaining why checking IsInsideToken is needed here.

JakeMerdichAMD marked an inline comment as done.May 15 2020, 8:51 AM

Rebase to fix merge conflict

This revision is now accepted and ready to land.May 17 2020, 3:38 AM

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

This revision was automatically updated to reflect the committed changes.