Fixed long-standing issue with incorrect length calculation of multi-line comments.

Summary:
A trailing block comment having multiple lines would cause extremely
high penalties if the summary length of its lines is more than the column limit.
Fixed by always considering only the last line of a multi-line block comment.
Removed a long-standing FIXME from relevant tests and added a motivating test
modelled after problem cases from real code.

Reviewers: klimek

Reviewed By: klimek

CC: cfe-commits
Differential Revision: http://llvm-reviews.chandlerc.com/D1010

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@184340 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp
index 14337e9760eb1f7081ba65b97969ea1aef064463..86437d4c18eb5927d4d99abe0e9406777ce36a1a 100644 (file)
--- a/lib/Format/Format.cpp
+++ b/lib/Format/Format.cpp
@@ -832,17 +832,17 @@ private:
     }
     if (Current.UnbreakableTailLength >= getColumnLimit())
       return 0;
-    unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
 
+    unsigned RemainingSpace = getColumnLimit() - Current.UnbreakableTailLength;
     bool BreakInserted = false;
     unsigned Penalty = 0;
-    unsigned PositionAfterLastLineInToken = 0;
+    unsigned RemainingTokenColumns = 0;
     for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
          LineIndex != EndIndex; ++LineIndex) {
       if (!DryRun)
         Token->replaceWhitespaceBefore(LineIndex, Whitespaces);
       unsigned TailOffset = 0;
-      unsigned RemainingTokenColumns = Token->getLineLengthAfterSplit(
+      RemainingTokenColumns = Token->getLineLengthAfterSplit(
           LineIndex, TailOffset, StringRef::npos);
       while (RemainingTokenColumns > RemainingSpace) {
         BreakableToken::Split Split =
@@ -868,11 +868,11 @@ private:
         RemainingTokenColumns = NewRemainingTokenColumns;
         BreakInserted = true;
       }
-      PositionAfterLastLineInToken = RemainingTokenColumns;
     }
 
+    State.Column = RemainingTokenColumns;
+
     if (BreakInserted) {
-      State.Column = PositionAfterLastLineInToken;
       // If we break the token inside a parameter list, we need to break before
       // the next parameter on all levels, so that the next parameter is clearly
       // visible. Line comments already introduce a break.

diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp
index 15a5d6b5746b13eb1c12b60ac5b4b9920c27b789..f20eca1119c1c00f3f7de432c6c344f4cf0179c2 100644 (file)
--- a/unittests/Format/FormatTest.cpp
+++ b/unittests/Format/FormatTest.cpp
@@ -755,7 +755,7 @@ TEST_F(FormatTest, RemovesTrailingWhitespaceOfComments) {
                    getLLVMStyleWithColumns(33)));
 }
 
-TEST_F(FormatTest, UnderstandsMultiLineComments) {
+TEST_F(FormatTest, UnderstandsBlockComments) {
   verifyFormat("f(/*test=*/ true);");
   EXPECT_EQ(
       "f(aaaaaaaaaaaaaaaaaaaaaaaaa, /* Trailing comment for aa... */\n"
@@ -786,7 +786,7 @@ TEST_F(FormatTest, UnderstandsMultiLineComments) {
                NoBinPacking);
 }
 
-TEST_F(FormatTest, AlignsMultiLineComments) {
+TEST_F(FormatTest, AlignsBlockComments) {
   EXPECT_EQ("/*\n"
             " * Really multi-line\n"
             " * comment.\n"
@@ -836,6 +836,13 @@ TEST_F(FormatTest, AlignsMultiLineComments) {
                    "        * line. */"));
 }
 
+TEST_F(FormatTest, CorrectlyHandlesLengthOfBlockComments) {
+  EXPECT_EQ("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+            "              aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */",
+            format("double *x; /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+                   "              aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */"));
+}
+
 TEST_F(FormatTest, SplitsLongCxxComments) {
   EXPECT_EQ("// A comment that\n"
             "// doesn't fit on\n"
@@ -919,24 +926,19 @@ TEST_F(FormatTest, PriorityOfCommentBreaking) {
 }
 
 TEST_F(FormatTest, MultiLineCommentsInDefines) {
-  // FIXME: The line breaks are still suboptimal (current guess
-  // is that this is due to the token length being misused), but
-  // the comment handling is correct.
-  EXPECT_EQ("#define A(      \\\n"
-            "    x) /*       \\\n"
-            "a comment       \\\n"
-            "inside */       \\\n"
+  EXPECT_EQ("#define A(x) /* \\\n"
+            "  a comment     \\\n"
+            "  inside */     \\\n"
             "  f();",
             format("#define A(x) /* \\\n"
                    "  a comment     \\\n"
                    "  inside */     \\\n"
                    "  f();",
                    getLLVMStyleWithColumns(17)));
-  EXPECT_EQ("#define A(x) /* \\\n"
-            "        a       \\\n"
-            "        comment \\\n"
-            "        inside  \\\n"
-            "        */      \\\n"
+  EXPECT_EQ("#define A(      \\\n"
+            "    x) /*       \\\n"
+            "  a comment     \\\n"
+            "  inside */     \\\n"
             "  f();",
             format("#define A(      \\\n"
                    "    x) /*       \\\n"