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

[krasimir]

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.

[krasimir]

• Add the original test cases

[krasimir]

ping

[djasper]

lib/Format/ContinuationIndenter.h

269

We should be *very* careful when adding stuff to ParenState as every extra bit of data and every extra comparison can have substantial cost.

Here, specifically:

• Make this more generic. This currently addresses a very specific use case (comment broken), whereas the action is probably always going to be the same (enforce break). I think we should call this "NoContinuation" or "NeedsNewline" or something.
• This seems always to only relate to the previous token. So it can be always reset when the state is moved further.
• As this always refers to the previous token, this can probably live in LineState instead of ParenState. That way, it has less runtime/space overhead.

[krasimir]

• Change bool from Paren to Line and generalize it

[krasimir]

ping

[djasper]

looks good.

lib/Format/BreakableToken.h

 ///   whitespace manager; note that the compressed whitespace may be in the
 ///   middle of the original line and of the reformatted line.
 ///
 /// For tokens where the whitespace before each line needs to be also
 /// reformatted, for example for tokens supporting reflow, there are analogous
 /// 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
 /// - replaceWhitespaceBefore, for executing the reflow using a whitespace
 ///   manager.
 ///
 /// For tokens that require the whitespace after the last line to be
 /// reformatted, for example in multiline jsdoc comments that require the
public:
   /// A result having offset == StringRef::npos means that no piece of the line
   /// needs to be reformatted before any breaks are made.
   virtual Split getSplitBefore(unsigned LineIndex, unsigned PreviousEndColumn,
                                unsigned ColumnLimit,
                                llvm::Regex &CommentPragmasRegex) const {
     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
   /// this line.
   virtual unsigned getLineLengthAfterSplitBefore(unsigned LineIndex,
                                                  unsigned TailOffset,
                                                  unsigned PreviousEndColumn,
                                                  unsigned ColumnLimit,
public:
 
   unsigned getLineLengthAfterSplit(unsigned LineIndex, unsigned TailOffset,
                                    StringRef::size_type Length) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) override;
   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,
                                          unsigned ColumnLimit,
                                          Split SplitBefore) const override;
   void replaceWhitespaceBefore(unsigned LineIndex, unsigned PreviousEndColumn,
                                unsigned ColumnLimit, Split SplitBefore,
                                WhitespaceManager &Whitespaces) override;

lib/Format/BreakableToken.cpp

unsigned ReflownColumn =
         getReflownColumn(TrimmedContent, LineIndex, PreviousEndColumn);
     if (ReflownColumn <= ColumnLimit) {
       return ReflownColumn;
     }
     return getLineLengthAfterSplit(LineIndex, TailOffset, StringRef::npos);
   }
 }
 
+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) {
   if (LineIndex == 0) {
     if (DelimitersOnNewline) {
       // Since we're breaking af index 1 below, the break position and the
       // break length are the same.
       size_t BreakLength = Lines[0].substr(1).find_first_not_of(Blanks);

lib/Format/ContinuationIndenter.h

struct ParenState {
   bool HasMultipleNestedBlocks : 1;
 
   // \brief The start of a nested block (e.g. lambda introducer in C++ or
   // "function" in JavaScript) is not wrapped to a new line.
   bool NestedBlockInlined : 1;
 
   bool operator<(const ParenState &Other) const {
     if (Indent != Other.Indent)
       return Indent < Other.Indent;
     if (LastSpace != Other.LastSpace)
       return LastSpace < Other.LastSpace;
     if (NestedBlockIndent != Other.NestedBlockIndent)
       return NestedBlockIndent < Other.NestedBlockIndent;
     if (FirstLessLess != Other.FirstLessLess)
       return FirstLessLess < Other.FirstLessLess;
     if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace)
       return BreakBeforeClosingBrace;
struct LineState {
   unsigned Column;
 
   /// \brief The token that needs to be next formatted.
   FormatToken *NextToken;
 
   /// \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;
 
   /// \brief The lowest \c NestingLevel on the current line.
   unsigned LowestLevelOnLine;
 
   /// \brief The start column of the string literal, if we're in a string
   /// literal sequence, 0 otherwise.
struct LineState {
   bool operator<(const LineState &Other) const {
     if (NextToken != Other.NextToken)
       return NextToken < Other.NextToken;
     if (Column != Other.Column)
       return Column < Other.Column;
     if (LineContainsContinuedForLoopSection !=
         Other.LineContainsContinuedForLoopSection)
       return LineContainsContinuedForLoopSection;
+    if (NoContinuation != Other.NoContinuation)
+      return NoContinuation;
     if (StartOfLineLevel != Other.StartOfLineLevel)
       return StartOfLineLevel < Other.StartOfLineLevel;
     if (LowestLevelOnLine != Other.LowestLevelOnLine)
       return LowestLevelOnLine < Other.LowestLevelOnLine;
     if (StartOfStringLiteral != Other.StartOfStringLiteral)
       return StartOfStringLiteral < Other.StartOfStringLiteral;
     if (IgnoreStackForComparison || Other.IgnoreStackForComparison)
       return false;
     return Stack < Other.Stack;
   }
 };
 
 } // end namespace format
 } // end namespace clang
 
 #endif

lib/Format/ContinuationIndenter.cpp

if (Style.IndentPPDirectives == FormatStyle::PPDIS_AfterHash &&
        Line->Type == LT_ImportStatement))
     State.Column = 0;
   State.Line = Line;
   State.NextToken = Line->First;
   State.Stack.push_back(ParenState(FirstIndent, FirstIndent,
                                    /*AvoidBinPacking=*/false,
                                    /*NoLineBreak=*/false));
   State.LineContainsContinuedForLoopSection = false;
+  State.NoContinuation = false;
   State.StartOfStringLiteral = 0;
   State.StartOfLineLevel = 0;
   State.LowestLevelOnLine = 0;
   State.IgnoreStackForComparison = false;
 
   if (Style.Language == FormatStyle::LK_TextProto) {
     // We need this in order to deal with the bin packing of text fields at
     // global scope.
if (Style.ColumnLimit != 0 && Previous.BlockKind == BK_Block &&
     return true;
 
   if (Current.is(tok::lessless) &&
       ((Previous.is(tok::identifier) && Previous.TokenText == "endl") ||
        (Previous.Tok.isLiteral() && (Previous.TokenText.endswith("\\n\"") ||
                                      Previous.TokenText == "\'\\n\'"))))
     return true;
 
+  if (Previous.is(TT_BlockComment) && Previous.IsMultiline)
+    return true;
+
+  if (State.NoContinuation)
+    return true;
+
   return false;
 }
 
 unsigned ContinuationIndenter::addTokenToState(LineState &State, bool Newline,
                                                bool DryRun,
                                                unsigned ExtraSpaces) {
   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() ==
             tok::pp_not_keyword))) {
     unsigned EndColumn =
         SourceMgr.getSpellingColumnNumber(Current.WhitespaceRange.getEnd());
     if (Current.LastNewlineOffset != 0) {
       // If there is a newline within this token, the final column will solely
if ((Text.endswith(Postfix = "\"") &&
         (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))) {
       Token.reset(new BreakableStringLiteral(Current, StartColumn, Prefix,
                                              Postfix, State.Line->InPPDirective,
                                              Encoding, Style));
     } else {
       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.
         switchesFormatting(Current))
       return addMultilineToken(Current, State);
     Token.reset(new BreakableBlockComment(
         Current, StartColumn, Current.OriginalColumn, !Current.Previous,
         State.Line->InPPDirective, Encoding, Style));
for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
     BreakableToken::Split SplitBefore(StringRef::npos, 0);
     if (ReflowInProgress) {
       SplitBefore = Token->getSplitBefore(LineIndex, RemainingTokenColumns,
                                           RemainingSpace, CommentPragmasRegex);
     }
     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);
     RemainingTokenColumns = Token->getLineLengthAfterSplitBefore(
         LineIndex, TailOffset, RemainingTokenColumns, ColumnLimit, SplitBefore);
     while (RemainingTokenColumns > RemainingSpace) {
       BreakableToken::Split Split = Token->getSplit(
           LineIndex, TailOffset, ColumnLimit, CommentPragmasRegex);
if (BreakInserted) {
     // 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.
     if (Current.isNot(TT_LineComment)) {
       for (unsigned i = 0, e = State.Stack.size(); i != e; ++i)
         State.Stack[i].BreakBeforeParameter = true;
     }
 
+    if (Current.is(TT_BlockComment))
+      State.NoContinuation = true;
+
     Penalty += Current.isStringLiteral() ? Style.PenaltyBreakString
                                          : Style.PenaltyBreakComment;
 
     State.Stack.back().LastSpace = StartColumn;
   }
 
   Token->updateNextToken(State);
 

unittests/Format/FormatTestComments.cpp

EXPECT_EQ("a = {\n"
             "    1111 /*      a\n"
             "          */\n"
             "};",
             format("a = {1111 /*      a */\n"
                    "};",
                    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"
                "  // b");
 }
 
 TEST_F(FormatTestComments, AlignTrailingComments) {
   EXPECT_EQ("#define MACRO(V)                       \\\n"

 "                                                                      * d*/",
       format(
 "/*                                                                */ /*\n"
 " *       a b\n"
 " *       c     d*/",
           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

unittests/Format/FormatTestJS.cpp

static void verifyFormat(
     std::string Result = format(Code, Style);
     EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
   }
 };
 
 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) {
   // Break the first line of a multiline jsdoc comment.
   EXPECT_EQ("/**\n"
             " * jsdoc line 1\n"
             " * jsdoc line 2\n"
             " */",