Implement more accurate penalty & trade-offs while breaking protruding tokens.

[klimek]

Summary

This is a counter-proposal to D33589.

[krasimir]

lib/Format/ContinuationIndenter.cpp

1517

I'd rather

DEBUG({
  if (ReflowInProgress)
    llvm::dbgs() << "  Reflowing.\n";
});

unittests/Format/FormatTest.cpp

8007

Why did the string got on a newline here?

unittests/Format/FormatTestComments.cpp

2452

Please also leave the old test case, which is about the */ just going over the column limit if not put on a newline.

2461

Why change 15 to 16? The same for the subsequent cases.

2493

Please also leave the old test case, which is about the */ just going over the column limit if not put on a newline.

2495–2507

Please also leave the old test case, which is about the */ just going over the column limit if not put on a newline.

[klimek]

Address review comments.

unittests/Format/FormatTest.cpp

8007

Because then we break inside the string less often? That seems clearly better than before?

unittests/Format/FormatTestComments.cpp

2461

Because we're breaking before the comment now (see above), which according to the doc I wrote you agree is better, but changing all the tests seemed to then test something completely different.

[krasimir]

lib/Format/ContinuationIndenter.cpp

1616

nit: remove an empty line

unittests/Format/FormatTest.cpp

9951

Why didn't this get reformatted as:

EXPECT_EQ("int a; /* first line\n"
          "        * second line\n"
          "        * third line\n"
          "        */",

[klimek]

Address review comments.

unittests/Format/FormatTest.cpp

9951

Added FIXME.

[klimek]

Just exporting

[Typz]

Generally, this indeed improves the situation (though I cannot say much about the code itself, it is still too subtle for my shallow knowledge of clang-format).

But it seems to give some strange looking result with long comments: it seems like the decision is made at each line (e.g. is it better to wrap this line or overflow a bit), so we can get a comment where each line overflows by a few characters, even if the total is worse... For exemple, say we have a 100 lines of comment, with 9 characters overflow on each line, and an excess character penalty of 30 : with this patch nothing will be re-wrapped (9*30 = 270 is less the the 300 penalty for wrapping); but the total penatly would be 900....

(btw, it seems this got merged, but the ticket does not reflect it)

lib/Format/BreakableToken.cpp

289

commented-out code to be removed?

296

unconditional return --> remove the end of the function

560

remove commented-out code

595

BreakableLineCommentSection

[krasimir]

looks like there are more changes needed.

[klimek]

In D40068#931679, @Typz wrote:

Generally, this indeed improves the situation (though I cannot say much about the code itself, it is still too subtle for my shallow knowledge of clang-format).

But it seems to give some strange looking result with long comments: it seems like the decision is made at each line (e.g. is it better to wrap this line or overflow a bit), so we can get a comment where each line overflows by a few characters, even if the total is worse... For exemple, say we have a 100 lines of comment, with 9 characters overflow on each line, and an excess character penalty of 30 : with this patch nothing will be re-wrapped (9*30 = 270 is less the the 300 penalty for wrapping); but the total penatly would be 900....

(btw, it seems this got merged, but the ticket does not reflect it)

Ugh, sorry, I think an arc patch I did without --create on a branch that was branched after the original patch was sent out broke phab :( :( This is not actually the state the patch was in when submitted, or the state it was LG'ed in (see https://reviews.llvm.org/rL318515 for the smaller patch)

In D40068#931679, @Typz wrote:

Generally, this indeed improves the situation (though I cannot say much about the code itself, it is still too subtle for my shallow knowledge of clang-format).

But it seems to give some strange looking result with long comments: it seems like the decision is made at each line (e.g. is it better to wrap this line or overflow a bit), so we can get a comment where each line overflows by a few characters, even if the total is worse... For exemple, say we have a 100 lines of comment, with 9 characters overflow on each line, and an excess character penalty of 30 : with this patch nothing will be re-wrapped (9*30 = 270 is less the the 300 penalty for wrapping); but the total penatly would be 900....

(btw, it seems this got merged, but the ticket does not reflect it)

[klimek]

Self-accepting and closing, as the underlying change has landed, and this diff is incorrect now.
I also have an idea to solve the problem Typz has brought up.

lib/Format/BreakableToken.h

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 {
+  /// \brief Returns whether there will be a line break at the start of the
+  /// token.
+  virtual bool introducesBreakBeforeToken() 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,
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;
+  bool introducesBreakBeforeToken() 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

BreakableComment::getReflowSplit(StringRef Text, StringRef ReflowPrefix,
   // If the full line fits up, we return a reflow split after it,
   // otherwise we compute the largest piece of text that fits after
   // ReflowStartColumn.
   Split ReflowSplit =
       FullWidth <= ColumnLimit
           ? Split(TrimmedText.size(), Text.size() - TrimmedText.size())
           : getCommentSplit(Text, ReflowStartColumn, ColumnLimit,
                             Style.TabWidth, Encoding);
 
   // We need to be extra careful here, because while it's OK to keep a long line
   // if it can't be broken into smaller pieces (like when the first word of a
   // long line is longer than the column limit), it's not OK to reflow that long
   // word up. So we recompute the size of the previous line after reflowing and
   // only return the reflow split if that's under the line limit.
   if (ReflowSplit.first != StringRef::npos &&
       // Check if the width of the newly reflown line is under the limit.
       PreviousEndColumn + ReflowPrefix.size() +
               encoding::columnWidthWithTabs(Text.substr(0, ReflowSplit.first),
                                             PreviousEndColumn +
                                                 ReflowPrefix.size(),
                                             Style.TabWidth, Encoding) <=
           ColumnLimit) {
     return ReflowSplit;
   }

 
 BreakableToken::Split BreakableBlockComment::getSplitBefore(
     unsigned LineIndex, unsigned PreviousEndColumn, unsigned ColumnLimit,
     llvm::Regex &CommentPragmasRegex) const {
   if (!mayReflow(LineIndex, CommentPragmasRegex))
     return Split(StringRef::npos, 0);
   StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks);
   Split Result = getReflowSplit(TrimmedContent, ReflowPrefix, PreviousEndColumn,
                                 ColumnLimit);
   // Result is relative to TrimmedContent. Adapt it relative to
   // Content[LineIndex].
   if (Result.first != StringRef::npos)
     Result.first += Content[LineIndex].size() - TrimmedContent.size();
   return Result;
 }
 
 unsigned
if (SplitBefore.first == StringRef::npos ||
       // Block comment line contents contain the trailing whitespace after the
       // decoration, so the need of left trim. Note that this behavior is
       // consistent with the breaking of block comments where the indentation of
       // a broken line is uniform across all the lines of the block comment.
       SplitBefore.first + SplitBefore.second <
           Content[LineIndex].ltrim().size()) {
     // A piece of line, not the whole, gets reflown.
     return getLineLengthAfterSplit(LineIndex, TailOffset, StringRef::npos);
   } else {
     // The whole line gets reflown, need to check if we need to insert a break
     // for the postfix or not.
     StringRef TrimmedContent = Content[LineIndex].ltrim(Blanks);
     unsigned ReflownColumn =
         getReflownColumn(TrimmedContent, LineIndex, PreviousEndColumn);
     if (ReflownColumn <= ColumnLimit) {
       return ReflownColumn;
     }
     return getLineLengthAfterSplit(LineIndex, TailOffset, StringRef::npos);
   }
 }
 
-bool BreakableBlockComment::introducesBreakBefore(unsigned LineIndex) const {
+bool BreakableBlockComment::introducesBreakBeforeToken() const {
   // A break is introduced when we want delimiters on newline.
-  return LineIndex == 0 && DelimitersOnNewline &&
+  return 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) {

lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken &Current,
   unsigned StartColumn = State.Column - Current.ColumnWidth;
   if (Current.is(TT_LineComment)) {
     // We don't insert backslashes when breaking line comments.
     ColumnLimit = Style.ColumnLimit;
   }
   if (Current.UnbreakableTailLength >= ColumnLimit)
     return 0;
 
+  unsigned NewBreakPenalty = Current.isStringLiteral()
+                                 ? Style.PenaltyBreakString
+                                 : Style.PenaltyBreakComment;
   unsigned RemainingSpace = ColumnLimit - Current.UnbreakableTailLength;
-  bool BreakInserted = false;
+  bool BreakInserted = Token->introducesBreakBeforeToken();
+  // Store whether we inserted a new line break at the end of the previous
+  // logical line.
+  bool NewBreakBefore = false;
   // We use a conservative reflowing strategy. Reflow starts after a line is
   // broken or the corresponding whitespace compressed. Reflow ends as soon as a
   // line that doesn't get reflown with the previous line is reached.
   bool ReflowInProgress = false;
   unsigned Penalty = 0;
   unsigned RemainingTokenColumns = 0;
   unsigned TailOffset = 0;
+  DEBUG(llvm::dbgs() << "Breaking protruding token at column " << StartColumn
+                     << ".\n");
   for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
        LineIndex != EndIndex; ++LineIndex) {
+    DEBUG(llvm::dbgs() << "  Line: " << LineIndex
+                       << " (Reflow: " << ReflowInProgress << ")\n");
     BreakableToken::Split SplitBefore(StringRef::npos, 0);
     if (ReflowInProgress) {
       SplitBefore = Token->getSplitBefore(LineIndex, RemainingTokenColumns,
                                           RemainingSpace, CommentPragmasRegex);
     }
     ReflowInProgress = SplitBefore.first != StringRef::npos;
+    DEBUG({
+      if (ReflowInProgress)
+        llvm::dbgs() << "  Reflowing.\n";
+    });
     TailOffset =
         ReflowInProgress ? (SplitBefore.first + SplitBefore.second) : 0;
-    BreakInserted = BreakInserted || Token->introducesBreakBefore(LineIndex);
+    // If we found a reflow split and have added a new break before this line,
+    // we are going to remove the line break at the start of the next logical
+    // line.
+    // For example, here we'll add a new line break after 'text', and
+    // subsequently delete the line break between 'that' and 'reflows'.
+    //   // some text that
+    //   // reflows
+    // ->
+    //   // some text
+    //   // that reflows
+    // When adding the line break, we also added the penalty for it, so we need
+    // to subtract that penalty again when we remove the line break due to
+    // reflowing.
+    if (ReflowInProgress && NewBreakBefore) {
+      assert(Penalty >= NewBreakPenalty);
+      Penalty -= NewBreakPenalty;
+    }
+    NewBreakBefore = false;
     if (!DryRun)
       Token->replaceWhitespaceBefore(LineIndex, RemainingTokenColumns,
                                      RemainingSpace, SplitBefore, Whitespaces);
     RemainingTokenColumns = Token->getLineLengthAfterSplitBefore(
         LineIndex, TailOffset, RemainingTokenColumns, ColumnLimit, SplitBefore);
     while (RemainingTokenColumns > RemainingSpace) {
+      DEBUG(llvm::dbgs() << "    Over limit, need: " << RemainingTokenColumns
+                         << ", space: " << RemainingSpace << "\n");
       BreakableToken::Split Split = Token->getSplit(
           LineIndex, TailOffset, ColumnLimit, CommentPragmasRegex);
       if (Split.first == StringRef::npos) {
         // The last line's penalty is handled in addNextStateToQueue().
         if (LineIndex < EndIndex - 1)
           Penalty += Style.PenaltyExcessCharacter *
                      (RemainingTokenColumns - RemainingSpace);
+        DEBUG(llvm::dbgs() << "    No break opportunity.\n");
         break;
       }
       assert(Split.first != 0);
 
       // Check if compressing the whitespace range will bring the line length
       // under the limit. If that is the case, we perform whitespace compression
       // instead of inserting a line break.
       unsigned RemainingTokenColumnsAfterCompression =
           Token->getLineLengthAfterCompression(RemainingTokenColumns, Split);
       if (RemainingTokenColumnsAfterCompression <= RemainingSpace) {
         RemainingTokenColumns = RemainingTokenColumnsAfterCompression;
         ReflowInProgress = true;
         if (!DryRun)
           Token->compressWhitespace(LineIndex, TailOffset, Split, Whitespaces);
+        DEBUG(llvm::dbgs() << "    Compressing below limit.\n");
+        break;
+      }
+
+      // Compute both the penalties for:
+      // - not breaking, and leaving excess characters
+      // - adding a new line break
+      assert(RemainingTokenColumnsAfterCompression > RemainingSpace);
+      unsigned ExcessCharactersPenalty =
+          (RemainingTokenColumnsAfterCompression - RemainingSpace) *
+          Style.PenaltyExcessCharacter;
+
+      unsigned BreakPenalty = NewBreakPenalty;
+      unsigned ColumnsUsed =
+          Token->getLineLengthAfterSplit(LineIndex, TailOffset, Split.first);
+      if (ColumnsUsed > ColumnLimit)
+        BreakPenalty +=
+            Style.PenaltyExcessCharacter * (ColumnsUsed - ColumnLimit);
+
+      DEBUG(llvm::dbgs() << "    Penalty excess: " << ExcessCharactersPenalty
+                         << "\n            break : " << BreakPenalty << "\n");
+      // Only continue to add the line break if the penalty of the excess
+      // characters is larger than the penalty of the line break.
+      // FIXME: This does not take into account when we can later remove the
+      // line break again due to a reflow.
+      if (ExcessCharactersPenalty < BreakPenalty) {
+        if (!DryRun)
+          Token->compressWhitespace(LineIndex, TailOffset, Split, Whitespaces);
+        // Do not set ReflowInProgress: we do not have any space left to
+        // reflow into.
+        Penalty += ExcessCharactersPenalty;
         break;
       }
 
       unsigned NewRemainingTokenColumns = Token->getLineLengthAfterSplit(
           LineIndex, TailOffset + Split.first + Split.second, StringRef::npos);
 
       // When breaking before a tab character, it may be moved by a few columns,
       // but will still be expanded to the next tab stop, so we don't save any
       // columns.
       if (NewRemainingTokenColumns == RemainingTokenColumns)
+        // FIXME: Do we need to adjust the penalty?
         break;
-
       assert(NewRemainingTokenColumns < RemainingTokenColumns);
+
       if (!DryRun)
         Token->insertBreak(LineIndex, TailOffset, Split, Whitespaces);
-      Penalty += Current.SplitPenalty;
-      unsigned ColumnsUsed =
-          Token->getLineLengthAfterSplit(LineIndex, TailOffset, Split.first);
-      if (ColumnsUsed > ColumnLimit) {
-        Penalty += Style.PenaltyExcessCharacter * (ColumnsUsed - ColumnLimit);
-      }
+
+      Penalty += BreakPenalty;
       TailOffset += Split.first + Split.second;
       RemainingTokenColumns = NewRemainingTokenColumns;
       ReflowInProgress = true;
       BreakInserted = true;
+      NewBreakBefore = true;
     }
   }
 
   BreakableToken::Split SplitAfterLastLine =
       Token->getSplitAfterLastLine(TailOffset, ColumnLimit);
   if (SplitAfterLastLine.first != StringRef::npos) {
+    DEBUG(llvm::dbgs() << "Replacing whitespace after last line.\n");
     if (!DryRun)
       Token->replaceWhitespaceAfterLastLine(TailOffset, SplitAfterLastLine,
                                             Whitespaces);
     RemainingTokenColumns = Token->getLineLengthAfterSplitAfterLastLine(
         TailOffset, SplitAfterLastLine);
   }
 
   State.Column = RemainingTokenColumns;
 
   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);
 
   return Penalty;
 }
 

unittests/Format/FormatTest.cpp

EXPECT_EQ("someFunction1234567890(\n"
             "    \"f\");",
             format("someFunction1234567890(\"aaabbbcccdddeeefff\");",
                    getLLVMStyleWithColumns(25)));
   EXPECT_EQ("someFunction1234567890(\n"
             "    \"aaabbbcccdddeeeff\"\n"
             "    \"f\");",
             format("someFunction1234567890(\"aaabbbcccdddeeefff\");",
                    getLLVMStyleWithColumns(24)));
-  EXPECT_EQ("someFunction(\"aaabbbcc \"\n"
-            "             \"ddde \"\n"
-            "             \"efff\");",
+  EXPECT_EQ("someFunction(\n"
+            "    \"aaabbbcc ddde \"\n"
+            "    \"efff\");",
             format("someFunction(\"aaabbbcc ddde efff\");",
                    getLLVMStyleWithColumns(25)));
   EXPECT_EQ("someFunction(\"aaabbbccc \"\n"
             "             \"ddeeefff\");",
             format("someFunction(\"aaabbbccc ddeeefff\");",
                    getLLVMStyleWithColumns(25)));
   EXPECT_EQ("someFunction1234567890(\n"
             "    \"aaabb \"\n"
             "    \"cccdddeeefff\");",
             format("someFunction1234567890(\"aaabb cccdddeeefff\");",
                    getLLVMStyleWithColumns(25)));
   EXPECT_EQ("#define A          \\\n"
             "  string s =       \\\n"
             "      \"123456789\"  \\\n"
             "      \"0\";         \\\n"
             "  int i;",
             format("#define A string s = \"1234567890\"; int i;",
                    getLLVMStyleWithColumns(20)));
-  // FIXME: Put additional penalties on breaking at non-whitespace locations.
-  EXPECT_EQ("someFunction(\"aaabbbcc \"\n"
-            "             \"dddeeeff\"\n"
-            "             \"f\");",
+  EXPECT_EQ("someFunction(\n"
+            "    \"aaabbbcc \"\n"
+            "    \"dddeeefff\");",
             format("someFunction(\"aaabbbcc dddeeefff\");",
                    getLLVMStyleWithColumns(25)));
 }
 
 TEST_F(FormatTest, DoNotBreakStringLiteralsInEscapeSequence) {
   EXPECT_EQ("\"\\a\"", format("\"\\a\"", getLLVMStyleWithColumns(3)));
   EXPECT_EQ("\"\\\"", format("\"\\\"", getLLVMStyleWithColumns(2)));
   EXPECT_EQ("\"test\"\n"

 }
 
 TEST_F(FormatTest, UnderstandPragmaOption) {
   verifyFormat("#pragma option -C -A");
 
   EXPECT_EQ("#pragma option -C -A", format("#pragma    option   -C   -A"));
 }
 
+TEST_F(FormatTest, OptimizeBreakPenaltyVsExcess) {
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 20;
+
+  verifyFormat("int a; // the\n"
+               "       // comment", Style);
+  EXPECT_EQ("int a; /* first line\n"
+            "        * second\n"
+            "        * line third\n"
+            "        * line\n"
+            "        */",
+            format("int a; /* first line\n"
+                   "        * second\n"
+                   "        * line third\n"
+                   "        * line\n"
+                   "        */",
+                   Style));
+  EXPECT_EQ("int a; // first line\n"
+            "       // second\n"
+            "       // line third\n"
+            "       // line",
+            format("int a; // first line\n"
+                   "       // second line\n"
+                   "       // third line",
+                   Style));
+
+  Style.PenaltyExcessCharacter = 90;
+  verifyFormat("int a; // the comment", Style);
+  EXPECT_EQ("int a; // the\n"
+            "       // comment aa",
+            format("int a; // the comment aa", Style));
+  EXPECT_EQ("int a; // first line\n"
+            "       // second line\n"
+            "       // third line",
+            format("int a; // first line\n"
+                   "       // second line\n"
+                   "       // third line",
+                   Style));
+  EXPECT_EQ("int a; /* first line\n"
+            "        * second line\n"
+            "        * third line\n"
+            "        */",
+            format("int a; /* first line\n"
+                   "        * second line\n"
+                   "        * third line\n"
+                   "        */",
+                   Style));
+  // FIXME: Investigate why this is not getting the same layout as the test
+  // above.
+  EXPECT_EQ("int a; /* first line\n"
+            "        * second\n"
+            "        * line third\n"
+            "        * line\n"
+            "        */",
+            format("int a; /* first line second line third line"
+                   "\n*/",
+                   Style));
+
+  EXPECT_EQ("// foo bar baz bazfoo\n"
+            "// foo bar\n",
+            format("// foo bar baz bazfoo\n"
+                   "// foo            bar\n",
+                   Style));
+  EXPECT_EQ("// foo bar baz bazfoo\n"
+            "// foo bar\n",
+            format("// foo bar baz      bazfoo\n"
+                   "// foo            bar\n",
+                   Style));
+
+  // FIXME: Optimally, we'd keep bazfoo on the first line and reflow bar to the
+  // next one.
+  EXPECT_EQ("// foo bar baz\n"
+            "// bazfoo bar foo\n"
+            "// bar\n",
+            format("// foo bar baz      bazfoo bar\n"
+                   "// foo            bar\n",
+                   Style));
+
+  EXPECT_EQ("// foo bar baz bazfoo\n"
+            "// foo bar baz\n"
+            "// bazfoo bar foo\n"
+            "// bar\n",
+            format("// foo bar baz      bazfoo\n"
+                   "// foo bar baz      bazfoo bar\n"
+                   "// foo bar\n",
+                   Style));
+
+  // FIXME: Optimally, we'd keep 'bar' in the last line at the end of the line,
+  // as it does not actually protrude far enough to make breaking pay off.
+  // Unfortunately, due to how reflowing is currently implemented, we already
+  // check the column limit after the reflowing decision and extend the reflow
+  // range, so we will not take whitespace compression into account.
+  EXPECT_EQ("// foo bar baz bazfoo\n"
+            "// foo bar baz\n"
+            "// bazfoo bar foo\n"
+            "// bar\n",
+            format("// foo bar baz      bazfoo\n"
+                   "// foo bar baz      bazfoo bar\n"
+                   "// foo           bar\n",
+                   Style));
+}
+
 #define EXPECT_ALL_STYLES_EQUAL(Styles)                                        \
   for (size_t i = 1; i < Styles.size(); ++i)                                   \
   EXPECT_EQ(Styles[0], Styles[i]) << "Style #" << i << " of " << Styles.size() \
                                   << " differs from Style #0"
 
 TEST_F(FormatTest, GetsPredefinedStyleByName) {
   SmallVector<FormatStyle, 3> Styles;
   Styles.resize(3);

unittests/Format/FormatTestComments.cpp

TEST_F(FormatTestComments, ReflowsComments) {
   // available space at current line.
   EXPECT_EQ("int i; // trigger\n"
             "       // reflow\n"
             "       // longsec\n",
             format("int i; // trigger reflow\n"
                    "       // longsec\n",
                    getLLVMStyleWithColumns(20)));
 
+  // Simple case that correctly handles reflow in parameter lists.
+  EXPECT_EQ("a = f(/* looooooooong\n"
+            "       * long long\n"
+            "       */\n"
+            "      a);",
+            format("a = f(/* looooooooong long\n* long\n*/ a);",
+                   getLLVMStyleWithColumns(22)));
+  // Tricky case that has fewer lines if we reflow the comment, ending up with
+  // fewer lines.
+  EXPECT_EQ("a = f(/* loooooong\n"
+            "       * long long\n"
+            "       */\n"
+            "      a);",
+            format("a = f(/* loooooong long\n* long\n*/ a);",
+                   getLLVMStyleWithColumns(22)));
+
   // Keep empty comment lines.
   EXPECT_EQ("/**/", format(" /**/", getLLVMStyleWithColumns(20)));
   EXPECT_EQ("/* */", format(" /* */", getLLVMStyleWithColumns(20)));
   EXPECT_EQ("/*  */", format(" /*  */", getLLVMStyleWithColumns(20)));
   EXPECT_EQ("//", format(" //  ", getLLVMStyleWithColumns(20)));
   EXPECT_EQ("///", format(" ///  ", getLLVMStyleWithColumns(20)));
 }
 
EXPECT_EQ("a = {\n"
             "};",
             format("a = {1111 /*      a */\n"
                    "};",
                    getLLVMStyleWithColumns(15)));
 }
 
 TEST_F(FormatTestComments, BreaksAfterMultilineBlockCommentsInParamLists) {
   EXPECT_EQ("a = f(/* long\n"
-            "         long\n"
-            "       */\n"
-            "      a);",
+            "         long */\n"
+            "      a);",
+            format("a = f(/* long long */ a);", getLLVMStyleWithColumns(16)));
+  EXPECT_EQ("a = f(\n"
+            "    /* long\n"
+            "       long */\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)));
+                   getLLVMStyleWithColumns(16)));
 
   EXPECT_EQ("a = f(/* long\n"
             "         long\n"
             "       */\n"
             "      a);",
             format("a = f(/* long\n"
                    "         long\n"
                    "       */ a);",
-                   getLLVMStyleWithColumns(15)));
+                   getLLVMStyleWithColumns(16)));
 
   EXPECT_EQ("a = f(/* long\n"
             "         long\n"
             "       */\n"
             "      (1 + 1));",
             format("a = f(/* long\n"
                    "         long\n"
                    "       */ (1 + 1));",
-                   getLLVMStyleWithColumns(15)));
+                   getLLVMStyleWithColumns(16)));
 
   EXPECT_EQ(
       "a = f(a,\n"
       "      /* long\n"
-      "         long\n"
-      "       */\n"
-      "      b);",
+      "         long */\n"
+      "      b);",
+      format("a = f(a, /* long long */ b);", getLLVMStyleWithColumns(16)));
+  EXPECT_EQ(
+      "a = f(\n"
+      "    a,\n"
+      "    /* long\n"
+      "       long */\n"
+      "    b);",
       format("a = f(a, /* long long */ b);", getLLVMStyleWithColumns(15)));
 
-  EXPECT_EQ(
-      "a = f(a,\n"
+  EXPECT_EQ("a = f(a,\n"
             "      /* long\n"
-      "         long\n"
-      "       */\n"
-      "      (1 + 1));",
-      format("a = f(a, /* long long */ (1 + 1));", getLLVMStyleWithColumns(15)));
+            "         long */\n"
+            "      (1 + 1));",
+            format("a = f(a, /* long long */ (1 + 1));",
+                   getLLVMStyleWithColumns(16)));
+  EXPECT_EQ("a = f(\n"
+            "    a,\n"
+            "    /* long\n"
+            "       long */\n"
+            "    (1 + 1));",
+            format("a = f(a, /* long long */ (1 + 1));",
+                   getLLVMStyleWithColumns(15)));
 }
 
 TEST_F(FormatTestComments, IndentLineCommentsInStartOfBlockAtEndOfFile) {
   verifyFormat("{\n"
                "  // a\n"
                "  // b");
 }