[clang-format] Break before next parameter after a formatted multiline raw string parameter

[krasimir]

Summary

Currently clang-format breaks before the next parameter after multiline parameters (also recursively for the parent expressions of multiline parameters). However, it fails to do so for formatted multiline raw string literals:

$ cat test.cc
// Examples

// Regular multiline tokens
int x = f(R"(multi
             line)", 2);
}

int y = g(h(R"(multi
              line)"), 2);

// Formatted multiline tokens
int z = f(R"pb(multi: 1  #
               line: 2)pb", 2);

int w = g(h(R"pb(multi: 1  #
                 line: 2)pb"), 2);
$ clang-format -style=google test.cc
// Examples

// Regular multiline tokens
int x = f(R"(multi
             line)",
          2);
}

int y = g(h(R"(multi
              line)"),
          2);

// Formatted multiline tokens
int z = f(R"pb(multi: 1  #
               line: 2)pb", 2);

int w = g(h(R"pb(multi: 1  #
                 line: 2)pb"), 2);

This patch addresses this inconsistency by forcing breaking after multiline formatted raw string literals. This requires a little tweak to the indentation chosen for the contents of a formatted raw string literal: in case when that's a parameter and not the last one, the indentation is based off of the uniform indentation of all of the parameters.

[krasimir]

• Add more tests and tidy-up

[sammccall]

lib/Format/ContinuationIndenter.cpp

1594 ↗

(On Diff #167749)

nit: why are you doing the multiline side-effect between computing the penalty and returning it?

[krasimir]

lib/Format/ContinuationIndenter.cpp

1594 ↗

(On Diff #167749)

Thank you!

[krasimir]

• Address review comment

cfe/trunk/lib/Format/ContinuationIndenter.cpp

unsigned ContinuationIndenter::reformatRawStringLiteral(
   //   - if the content starts on newline, it is one level more than the current
   //     indent, and
   //   - if the content does not start on a newline, it is the first start
   //     column.
   // These rules have the advantage that the formatted content both does not
   // violate the rectangle rule and visually flows within the surrounding
   // source.
   bool ContentStartsOnNewline = Current.TokenText[OldPrefixSize] == '\n';
-  unsigned NextStartColumn =
-      ContentStartsOnNewline
-          ? State.Stack.back().NestedBlockIndent + Style.IndentWidth
+  // If this token is the last parameter (checked by looking if it's followed by
+  // `)`, the base the indent off the line's nested block indent. Otherwise,
+  // base the indent off the arguments indent, so we can achieve:
+  // fffffffffff(1, 2, 3, R"pb(
+  //     key1: 1  #
+  //     key2: 2)pb");
+  //
+  // fffffffffff(1, 2, 3,
+  //             R"pb(
+  //               key1: 1  #
+  //               key2: 2
+  //             )pb",
+  //             5);
+  unsigned CurrentIndent = (Current.Next && Current.Next->is(tok::r_paren))
+                               ? State.Stack.back().NestedBlockIndent
+                               : State.Stack.back().Indent;
+  unsigned NextStartColumn = ContentStartsOnNewline
+                                 ? CurrentIndent + Style.IndentWidth
                                  : FirstStartColumn;
 
   // The last start column is the column the raw string suffix starts if it is
   // put on a newline.
   // The last start column is the intended indentation of the raw string postfix
   // if it is put on a newline. It is determined by the following rules:
   //   - if the raw string prefix starts on a newline, it is the column where
   //     that raw string prefix starts, and
   //   - if the raw string prefix does not start on a newline, it is the current
   //     indent.
   unsigned LastStartColumn = Current.NewlinesBefore
                                  ? FirstStartColumn - NewPrefixSize
-                                 : State.Stack.back().NestedBlockIndent;
+                                 : CurrentIndent;
 
   std::pair<tooling::Replacements, unsigned> Fixes = internal::reformat(
       RawStringStyle, RawText, {tooling::Range(0, RawText.size())},
       FirstStartColumn, NextStartColumn, LastStartColumn, "<stdin>",
       /*Status=*/nullptr);
 
   auto NewCode = applyAllReplacements(RawText, Fixes.first);
   tooling::Replacements NoFixes;
   if (!NewCode) {
-    State.Column += Current.ColumnWidth;
-    return 0;
+    return addMultilineToken(Current, State);
   }
   if (!DryRun) {
     if (NewDelimiter != OldDelimiter) {
       // In 'R"delimiter(...', the delimiter starts 2 characters after the start
       // of the token.
       SourceLocation PrefixDelimiterStart =
           Current.Tok.getLocation().getLocWithOffset(2);
       auto PrefixErr = Whitespaces.addReplacement(tooling::Replacement(
unsigned RawLastLineEndColumn = getLastLineEndColumn(
       *NewCode, FirstStartColumn, Style.TabWidth, Encoding);
   State.Column = RawLastLineEndColumn + NewSuffixSize;
   // Since we're updating the column to after the raw string literal here, we
   // have to manually add the penalty for the prefix R"delim( over the column
   // limit.
   unsigned PrefixExcessCharacters =
       StartColumn + NewPrefixSize > Style.ColumnLimit ?
       StartColumn + NewPrefixSize - Style.ColumnLimit : 0;
+  bool IsMultiline =
+      ContentStartsOnNewline || (NewCode->find('\n') != std::string::npos);
+  if (IsMultiline) {
+    // Break before further function parameters on all levels.
+    for (unsigned i = 0, e = State.Stack.size(); i != e; ++i)
+      State.Stack[i].BreakBeforeParameter = true;
+  }
   return Fixes.second + PrefixExcessCharacters * Style.PenaltyExcessCharacter;
 }
 
 unsigned ContinuationIndenter::addMultilineToken(const FormatToken &Current,
                                                  LineState &State) {
   // Break before further function parameters on all levels.
   for (unsigned i = 0, e = State.Stack.size(); i != e; ++i)
     State.Stack[i].BreakBeforeParameter = true;

cfe/trunk/unittests/Format/FormatTestRawStrings.cpp

auto S = R"pb(item_1:1,item_2:2)pb"+rest;)test",
 
   expect_eq(R"test(
 auto S =
     R"pb(item_1: 1 item_2: 2)pb" + rest;)test",
             format(R"test(
 auto S = R"pb(item_1:1 item_2:2)pb"+rest;)test",
                    getRawStringPbStyleWithColumns(40)));
 
+  // `rest` fits on the line after )pb", but forced on newline since the raw
+  // string literal is multiline.
   expect_eq(R"test(
 auto S = R"pb(item_1: 1,
               item_2: 2,
-              item_3: 3)pb" + rest;)test",
+              item_3: 3)pb" +
+         rest;)test",
             format(R"test(
 auto S = R"pb(item_1:1,item_2:2,item_3:3)pb"+rest;)test",
                    getRawStringPbStyleWithColumns(40)));
 
   expect_eq(R"test(
 auto S = R"pb(item_1: 1,
               item_2: 2,
               item_3: 3)pb" +
auto S = first + R"pb(item_1: 1
                       item_2: 2)pb";)test",
             format(R"test(
 auto S = first+R"pb(item_1:1 item_2:2)pb";)test",
                    getRawStringPbStyleWithColumns(40)));
 
   expect_eq(R"test(
 auto S = R"pb(item_1: 1,
               item_2: 2,
-              item_3: 3)pb" + rest;)test",
+              item_3: 3)pb" +
+         rest;)test",
             format(R"test(
 auto S = R"pb(item_1:1,item_2:2,item_3:3)pb"+rest;)test",
                    getRawStringPbStyleWithColumns(40)));
 
   expect_eq(R"test(
 auto S = R"pb(item_1: 1,
               item_2: 2,
               item_3: 3)pb" +

 item {
   key: value
 }
 )Raw"));
 })test",
                    Style));
 }
 
+TEST_F(FormatTestRawStrings,
+       BreaksBeforeNextParamAfterMultilineRawStringParam) {
+  FormatStyle Style = getRawStringPbStyleWithColumns(60);
+  expect_eq(R"test(
+int f() {
+  int a = g(x, R"pb(
+              key: 1  #
+              key: 2
+            )pb",
+            3, 4);
+}
+)test",
+            format(R"test(
+int f() {
+  int a = g(x, R"pb(
+              key: 1 #
+              key: 2
+            )pb", 3, 4);
+}
+)test",
+                   Style));
+
+  // Breaks after a parent of a multiline param.
+  expect_eq(R"test(
+int f() {
+  int a = g(x, h(R"pb(
+              key: 1  #
+              key: 2
+            )pb"),
+            3, 4);
+}
+)test",
+            format(R"test(
+int f() {
+  int a = g(x, h(R"pb(
+              key: 1 #
+              key: 2
+            )pb"), 3, 4);
+}
+)test",
+                   Style));
+  
+  expect_eq(R"test(
+int f() {
+  int a = g(x,
+            h(R"pb(
+                key: 1  #
+                key: 2
+              )pb",
+              2),
+            3, 4);
+}
+)test",
+            format(R"test(
+int f() {
+  int a = g(x, h(R"pb(
+              key: 1 #
+              key: 2
+            )pb", 2), 3, 4);
+}
+)test",
+                   Style));
+  // Breaks if formatting introduces a multiline raw string.
+  expect_eq(R"test(
+int f() {
+  int a = g(x, R"pb(key1: value111111111
+                    key2: value2222222222)pb",
+            3, 4);
+}
+)test",
+            format(R"test(
+int f() {
+  int a = g(x, R"pb(key1: value111111111 key2: value2222222222)pb", 3, 4);
+}
+)test",
+                   Style));
+  // Does not force a break after an original multiline param that is
+  // reformatterd as on single line.
+  expect_eq(R"test(
+int f() {
+  int a = g(R"pb(key: 1)pb", 2);
+})test",
+            format(R"test(
+int f() {
+  int a = g(R"pb(key:
+                 1)pb", 2);
+})test", Style));
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang