[clang-format] Break non-trailing comments, try 2

Summary:
This patch enables `BreakableToken` to manage the formatting of non-trailing
block comments. It is a refinement of https://reviews.llvm.org/D37007.
We discovered that the optimizer outsmarts us on cases where breaking the comment
costs considerably less than breaking after the comment. This patch addresses
this by ensuring that a newline is inserted between a block comment and the next
token.

Reviewers: djasper

Reviewed By: djasper

Subscribers: cfe-commits, klimek

Differential Revision: https://reviews.llvm.org/D37695

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

diff --git a/lib/Format/BreakableToken.cpp b/lib/Format/BreakableToken.cpp
index 8031ab9cd2843aa1eacdc5b36dfa574be8873587..bbf3d88eec45556b34025d735afc4f9913b52599 100644 (file)
--- a/lib/Format/BreakableToken.cpp
+++ b/lib/Format/BreakableToken.cpp
@@ -599,6 +599,12 @@ unsigned BreakableBlockComment::getLineLengthAfterSplitBefore(
   }
 }
 
+bool BreakableBlockComment::introducesBreakBefore(unsigned LineIndex) const {
+  // A break is introduced when we want delimiters on newline.
+  return LineIndex == 0 && DelimitersOnNewline &&
+         Lines[0].substr(1).find_first_not_of(Blanks) != StringRef::npos;
+}
+
 void BreakableBlockComment::replaceWhitespaceBefore(
     unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit,
     Split SplitBefore, WhitespaceManager &Whitespaces) {

diff --git a/lib/Format/BreakableToken.h b/lib/Format/BreakableToken.h
index 630e927e8fc237715ef135bf41bc4f21f8446720..8c2dc741d1e0bb7516d3c6cee20b02d06b39c765 100644 (file)
--- a/lib/Format/BreakableToken.h
+++ b/lib/Format/BreakableToken.h
@@ -58,6 +58,8 @@ struct FormatStyle;
 /// operations that might be executed before the main line breaking occurs:
 /// - getSplitBefore, for finding a split such that the content preceding it
 ///   needs to be specially reflown,
+/// - introducesBreakBefore, for checking if reformatting the beginning
+///   of the content introduces a line break before it,
 /// - getLineLengthAfterSplitBefore, for calculating the line length in columns
 ///   of the remainder of the content after the beginning of the content has
 ///   been reformatted, and
@@ -135,6 +137,12 @@ public:
     return Split(StringRef::npos, 0);
   }
 
+  /// \brief Returns if a break before the content at \p LineIndex will be
+  /// inserted after the whitespace preceding the content has been reformatted.
+  virtual bool introducesBreakBefore(unsigned LineIndex) const {
+    return false;
+  }
+
   /// \brief Returns the number of columns required to format the piece of line
   /// at \p LineIndex after the content preceding the whitespace range specified
   /// \p SplitBefore has been reformatted, but before any breaks are made to
@@ -339,6 +347,7 @@ public:
   Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn,
                        unsigned ColumnLimit,
                        llvm::Regex &CommentPragmasRegex) const override;
+  bool introducesBreakBefore(unsigned LineIndex) const override;
   unsigned getLineLengthAfterSplitBefore(unsigned LineIndex,
                                          unsigned TailOffset,
                                          unsigned PreviousEndColumn,

diff --git a/lib/Format/ContinuationIndenter.cpp b/lib/Format/ContinuationIndenter.cpp
index b438a3f64e71335fd168d43161bd0a12bae8be4e..b57b8de2e702251334a863f6fbeb9e5597558697 100644 (file)
--- a/lib/Format/ContinuationIndenter.cpp
+++ b/lib/Format/ContinuationIndenter.cpp
@@ -106,6 +106,7 @@ LineState ContinuationIndenter::getInitialState(unsigned FirstIndent,
                                    /*AvoidBinPacking=*/false,
                                    /*NoLineBreak=*/false));
   State.LineContainsContinuedForLoopSection = false;
+  State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
@@ -322,6 +323,12 @@ bool ContinuationIndenter::mustBreak(const LineState &State) {
                                      Previous.TokenText == "\'\\n\'"))))
     return true;
 
+  if (Previous.is(TT_BlockComment) && Previous.IsMultiline)
+    return true;
+
+  if (State.NoContinuation)
+    return true;
+
   return false;
 }
 
@@ -331,6 +338,8 @@ unsigned ContinuationIndenter::addTokenToState(LineState &State, bool Newline,
   const FormatToken &Current = *State.NextToken;
 
   assert(!State.Stack.empty());
+  State.NoContinuation = false;
+
   if ((Current.is(TT_ImplicitStringLiteral) &&
        (Current.Previous->Tok.getIdentifierInfo() == nullptr ||
         Current.Previous->Tok.getIdentifierInfo()->getPPKeywordID() ==
@@ -1286,7 +1295,7 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
       return 0;
     }
   } else if (Current.is(TT_BlockComment)) {
-    if (!Current.isTrailingComment() || !Style.ReflowComments ||
+    if (!Style.ReflowComments ||
         // If a comment token switches formatting, like
         // /* clang-format on */, we don't want to break it further,
         // but we may still want to adjust its indentation.
@@ -1332,6 +1341,7 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
     ReflowInProgress = SplitBefore.first != StringRef::npos;
     TailOffset =
         ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
+    BreakInserted = BreakInserted || Token->introducesBreakBefore(LineIndex);
     if (!DryRun)
       Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
                                      RemainingSpace, SplitBefore, Whitespaces);
@@ -1408,6 +1418,9 @@ unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
         State.Stack[i].BreakBeforeParameter = true;
     }
 
+    if (Current.is(TT_BlockComment))
+      State.NoContinuation = true;
+
     Penalty += Current.isStringLiteral() ? Style.PenaltyBreakString
                                          : Style.PenaltyBreakComment;
 

diff --git a/lib/Format/ContinuationIndenter.h b/lib/Format/ContinuationIndenter.h
index 9a06aa6f626721efb004965aa5479a148d53e9eb..631129c1ea1d208636b1d372a465c970d9d467ed 100644 (file)
--- a/lib/Format/ContinuationIndenter.h
+++ b/lib/Format/ContinuationIndenter.h
@@ -318,6 +318,9 @@ struct LineState {
   /// \brief \c true if this line contains a continued for-loop section.
   bool LineContainsContinuedForLoopSection;
 
+  /// \brief \c true if \p NextToken should not continue this line.
+  bool NoContinuation;
+
   /// \brief The \c NestingLevel at the start of this line.
   unsigned StartOfLineLevel;
 
@@ -364,6 +367,8 @@ struct LineState {
     if (LineContainsContinuedForLoopSection !=
         Other.LineContainsContinuedForLoopSection)
       return LineContainsContinuedForLoopSection;
+    if (NoContinuation != Other.NoContinuation)
+      return NoContinuation;
     if (StartOfLineLevel != Other.StartOfLineLevel)
       return StartOfLineLevel < Other.StartOfLineLevel;
     if (LowestLevelOnLine != Other.LowestLevelOnLine)

diff --git a/unittests/Format/FormatTestComments.cpp b/unittests/Format/FormatTestComments.cpp
index c8ebebc58ad491ef2686a84e39d4d23e941d071d..ceccc83587951faed04b86bb989db7addf3ea969 100644 (file)
--- a/unittests/Format/FormatTestComments.cpp
+++ b/unittests/Format/FormatTestComments.cpp
@@ -2407,6 +2407,57 @@ TEST_F(FormatTestComments, BlockCommentsAtEndOfLine) {
                    getLLVMStyleWithColumns(15)));
 }
 
+TEST_F(FormatTestComments, BreaksAfterMultilineBlockCommentsInParamLists) {
+  EXPECT_EQ("a = f(/* long\n"
+            "         long\n"
+            "       */\n"
+            "      a);",
+            format("a = f(/* long long */ a);", getLLVMStyleWithColumns(15)));
+
+  EXPECT_EQ("a = f(/* long\n"
+            "         long\n"
+            "       */\n"
+            "      a);",
+            format("a = f(/* long\n"
+                   "         long\n"
+                   "       */a);",
+                   getLLVMStyleWithColumns(15)));
+
+  EXPECT_EQ("a = f(/* long\n"
+            "         long\n"
+            "       */\n"
+            "      a);",
+            format("a = f(/* long\n"
+                   "         long\n"
+                   "       */ a);",
+                   getLLVMStyleWithColumns(15)));
+
+  EXPECT_EQ("a = f(/* long\n"
+            "         long\n"
+            "       */\n"
+            "      (1 + 1));",
+            format("a = f(/* long\n"
+                   "         long\n"
+                   "       */ (1 + 1));",
+                   getLLVMStyleWithColumns(15)));
+
+  EXPECT_EQ(
+      "a = f(a,\n"
+      "      /* long\n"
+      "         long\n"
+      "       */\n"
+      "      b);",
+      format("a = f(a, /* long long */ b);", getLLVMStyleWithColumns(15)));
+
+  EXPECT_EQ(
+      "a = f(a,\n"
+      "      /* long\n"
+      "         long\n"
+      "       */\n"
+      "      (1 + 1));",
+      format("a = f(a, /* long long */ (1 + 1));", getLLVMStyleWithColumns(15)));
+}
+
 TEST_F(FormatTestComments, IndentLineCommentsInStartOfBlockAtEndOfFile) {
   verifyFormat("{\n"
                "  // a\n"
@@ -2805,6 +2856,22 @@ TEST_F(FormatTestComments, NoCrush_Bug34236) {
           getLLVMStyleWithColumns(80)));
   // clang-format on
 }
+
+TEST_F(FormatTestComments, NonTrailingBlockComments) {
+  verifyFormat("const /** comment comment */ A = B;",
+               getLLVMStyleWithColumns(40));
+
+  verifyFormat("const /** comment comment comment */ A =\n"
+               "    B;",
+               getLLVMStyleWithColumns(40));
+
+  EXPECT_EQ("const /** comment comment comment\n"
+            "         comment */\n"
+            "    A = B;",
+            format("const /** comment comment comment comment */\n"
+                   "    A = B;",
+                   getLLVMStyleWithColumns(40)));
+}
 } // end namespace
 } // end namespace format
 } // end namespace clang

diff --git a/unittests/Format/FormatTestJS.cpp b/unittests/Format/FormatTestJS.cpp
index 4eaa26aaec6f2e0a276793a74df87066df048daf..3df01663e9d7b2640978812bcfdd6cb6aad5a685 100644 (file)
--- a/unittests/Format/FormatTestJS.cpp
+++ b/unittests/Format/FormatTestJS.cpp
@@ -65,6 +65,27 @@ protected:
 TEST_F(FormatTestJS, BlockComments) {
   verifyFormat("/* aaaaaaaaaaaaa */ aaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");
+  // Breaks after a single line block comment.
+  EXPECT_EQ("aaaaa = bbbb.ccccccccccccccc(\n"
+            "    /** @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */\n"
+            "    mediaMessage);",
+            format("aaaaa = bbbb.ccccccccccccccc(\n"
+                   "    /** "
+                   "@type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala} */ "
+                   "mediaMessage);",
+                   getGoogleJSStyleWithColumns(70)));
+  // Breaks after a multiline block comment.
+  EXPECT_EQ(
+      "aaaaa = bbbb.ccccccccccccccc(\n"
+      "    /**\n"
+      "     * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n"
+      "     */\n"
+      "    mediaMessage);",
+      format("aaaaa = bbbb.ccccccccccccccc(\n"
+             "    /**\n"
+             "     * @type_{!cccc.rrrrrrr.MMMMMMMMMMMM.LLLLLLLLLLL.lala}\n"
+             "     */ mediaMessage);",
+             getGoogleJSStyleWithColumns(70)));
 }
 
 TEST_F(FormatTestJS, JSDocComments) {