[clang-format] Ignore UnbreakableTailLength sometimes during breaking

[krasimir]

Summary

This patch fixes an issue where the UnbreakableTailLength would be counted towards
the length of a token during breaking, even though we can break after the token.

For example, this proto text with column limit 20

# ColumnLimit: 20  V
foo: {
  bar: {
    bazoo: "aaaaaaa"
  }
}

was broken:

# ColumnLimit: 20  V
foo: {
  bar: {
    bazoo:
        "aaaaaaa"
  }
}

because the 2 closing } were counted towards the string literal's UnbreakableTailLength.

[djasper]

lib/Format/ContinuationIndenter.cpp

1579 ↗

(On Diff #130897)

I think a comment might help here. Specifically, it should mention that this is required for the special case where there is an unbreakable tail only if certain other formatting decisions have been taken. The UnbreakableTailLength is an overapproximation in that case and we need to be correct here.

Thinking about this some more, there might actually be cases where this is still not correct as the unbreakable tail is neither 0 nor the precomputed value.

E.g. can we construct a case where there is a trailing comma in a braced list? Maybe this:

vector<string> x = {"aaaaaa",};

In this case, I think the comma will always be part of the string literal, but the "};" only get part of the tail if we don't wrap after the "{".

[krasimir]

• Address review comments

[krasimir]

lib/Format/ContinuationIndenter.cpp

1579 ↗

(On Diff #130897)

In the vector<string> x = {"aaaaa",}; only the comma is part of the UnbreakableTailLength of the string:

M=1 C=1 T=Unknown S=0 B=0 BK=0 P=59 Name=string_literal L=100 PPK=2 FakeLParens=1/ FakeRParens=0 Text='"aaaaa"'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=41 Name=comma L=101 PPK=2 FakeLParens= FakeRParens=1 Text=','
M=1 C=1 T=Unknown S=1 B=0 BK=0 P=41 Name=r_brace L=181 PPK=2 FakeLParens= FakeRParens=1 Text='}'
M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=182 PPK=2 FakeLParens= FakeRParens=0 Text=';'

breakProtrudingToken: CanBreak=0, UnbreakableTailLength=1

[djasper]

lib/Format/ContinuationIndenter.cpp

1579 ↗

(On Diff #130897)

Ok. The comma works around this by forcing a break before the "}". But what about:

vector<string> x = {"abc" /*comment*/};

The comment should be an unbreakable tail.

[krasimir]

lib/Format/ContinuationIndenter.cpp

1579 ↗

(On Diff #130897)

As you discovered, the comment-in-tail case is special-cased in the unbreakable tail computation, so this is not an issue.

[djasper]

Happy to go forward with this. I think we might also wanna investigate whether entirely removing UnbreakableTailLength would be beneficial. I think we implemented it as an optimization, but I can actually imagine it saving much. Plus the code would be simpler and we would conserve some memory.

[krasimir]

• Update comment

cfe/trunk/lib/Format/BreakableToken.h

 class BreakableStringLiteral : public BreakableToken {
 public:
   /// \brief Creates a breakable token for a single line string literal.
   ///
   /// \p StartColumn specifies the column in which the token will start
   /// after formatting.
   BreakableStringLiteral(const FormatToken &Tok, unsigned StartColumn,
                          StringRef Prefix, StringRef Postfix,
-                         bool InPPDirective, encoding::Encoding Encoding,
-                         const FormatStyle &Style);
+                         unsigned UnbreakableTailLength, bool InPPDirective,
+                         encoding::Encoding Encoding, const FormatStyle &Style);
 
   Split getSplit(unsigned LineIndex, unsigned TailOffset, unsigned ColumnLimit,
                  unsigned ReflowColumn,
                  llvm::Regex &CommentPragmasRegex) const override;
   void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
                    WhitespaceManager &Whitespaces) const override;
   void compressWhitespace(unsigned LineIndex, unsigned TailOffset, Split Split,
                           WhitespaceManager &Whitespaces) const override {}

cfe/trunk/lib/Format/BreakableToken.cpp

 
 unsigned BreakableStringLiteral::getContentStartColumn(unsigned LineIndex,
                                                        bool Break) const {
   return StartColumn + Prefix.size();
 }
 
 BreakableStringLiteral::BreakableStringLiteral(
     const FormatToken &Tok, unsigned StartColumn, StringRef Prefix,
-    StringRef Postfix, bool InPPDirective, encoding::Encoding Encoding,
-    const FormatStyle &Style)
+    StringRef Postfix, unsigned UnbreakableTailLength, bool InPPDirective,
+    encoding::Encoding Encoding, const FormatStyle &Style)
     : BreakableToken(Tok, InPPDirective, Encoding, Style),
       StartColumn(StartColumn), Prefix(Prefix), Postfix(Postfix),
-      UnbreakableTailLength(Tok.UnbreakableTailLength) {
+      UnbreakableTailLength(UnbreakableTailLength) {
   assert(Tok.TokenText.startswith(Prefix) && Tok.TokenText.endswith(Postfix));
   Line = Tok.TokenText.substr(
       Prefix.size(), Tok.TokenText.size() - Prefix.size() - Postfix.size());
 }
 
 BreakableToken::Split BreakableStringLiteral::getSplit(
     unsigned LineIndex, unsigned TailOffset, unsigned ColumnLimit,
     unsigned ContentStartColumn, llvm::Regex &CommentPragmasRegex) const {

cfe/trunk/lib/Format/ContinuationIndenter.cpp

if (Current.isStringLiteral()) {
     // reduce the overhead) for each FormatToken, which is a string, so that we
     // don't run multiple checks here on the hot path.
     if ((Text.endswith(Postfix = "\"") &&
          (Text.startswith(Prefix = "@\"") || Text.startswith(Prefix = "\"") ||
           Text.startswith(Prefix = "u\"") || Text.startswith(Prefix = "U\"") ||
           Text.startswith(Prefix = "u8\"") ||
           Text.startswith(Prefix = "L\""))) ||
         (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))) {
+      // We need this to address the case where there is an unbreakable tail
+      // only if certain other formatting decisions have been taken. The
+      // UnbreakableTailLength of Current is an overapproximation is that case
+      // and we need to be correct here.
+      unsigned UnbreakableTailLength = (State.NextToken && canBreak(State))
+                                           ? 0
+                                           : Current.UnbreakableTailLength;
       return llvm::make_unique<BreakableStringLiteral>(
-          Current, StartColumn, Prefix, Postfix, State.Line->InPPDirective,
-          Encoding, Style);
+          Current, StartColumn, Prefix, Postfix, UnbreakableTailLength,
+          State.Line->InPPDirective, Encoding, Style);
     }
   } else if (Current.is(TT_BlockComment)) {
     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 nullptr;

cfe/trunk/unittests/Format/FormatTestTextProto.cpp

verifyFormat("app_id: 'com.javax.swing.salsa.latino'\n"
                "product_data <product <1>>");
 
   verifyFormat("app <\n"
                "  app_id: 'com.javax.swing.salsa.latino'\n"
                "  headheadheadheadheadhead_id: 1\n"
                "  product_data <product {1}>\n"
                ">");
 }
+
+TEST_F(FormatTestTextProto, DiscardsUnbreakableTailIfCanBreakAfter) {
+  // The two closing braces count towards the string UnbreakableTailLength, but
+  // since we have broken after the corresponding opening braces, we don't
+  // consider that length for string breaking.
+  verifyFormat(
+      "foo: {\n"
+      "  bar: {\n"
+      "    text: \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\n"
+      "  }\n"
+      "}");
+}
 } // end namespace tooling
 } // end namespace clang