[clang-format] Don't always break before << between string literals

[owenca]

Instead, leave the line wrapping as is.

Fixes #43887.
Fixes #44363.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Instead, leave the line wrapping as is.

Fixes #43887.
Fixes #44363.

---

Full diff: https://github.com/llvm/llvm-project/pull/92214.diff

2 Files Affected:

• (modified) clang/lib/Format/TokenAnnotator.cpp (+5-2)
• (modified) clang/unittests/Format/FormatTest.cpp (+11)

diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index d0aa0838423e4..7c4c76a91f2c5 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5601,10 +5601,13 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
     return true;
   if (Left.IsUnterminatedLiteral)
     return true;
-  if (Right.is(tok::lessless) && AfterRight && Left.is(tok::string_literal) &&
+
+  if (BeforeLeft && BeforeLeft->is(tok::lessless) &&
+      Left.is(tok::string_literal) && Right.is(tok::lessless) && AfterRight &&
       AfterRight->is(tok::string_literal)) {
-    return true;
+    return Right.NewlinesBefore > 0;
   }
+
   if (Right.is(TT_RequiresClause)) {
     switch (Style.RequiresClausePosition) {
     case FormatStyle::RCPS_OwnLine:
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index e6f8e4a06515e..6f57f10e12e88 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -10539,6 +10539,17 @@ TEST_F(FormatTest, KeepStringLabelValuePairsOnALine) {
       "                  bbbbbbbbbbbbbbbbbbbbbbb);");
 }
 
+TEST_F(FormatTest, WrapBeforeInsertionOperatorbetweenStringLiterals) {
+  verifyFormat("QStringList() << \"foo\" << \"bar\";");
+
+  verifyNoChange("QStringList() << \"foo\"\n"
+                 "              << \"bar\";");
+
+  verifyFormat("log_error(log, \"foo\" << \"bar\");",
+               "log_error(log, \"foo\"\n"
+               "                   << \"bar\");");
+}
+
 TEST_F(FormatTest, UnderstandsEquals) {
   verifyFormat(
       "aaaaaaaaaaaaaaaaa =\n"

[owenca]

See also the discussion in https://reviews.llvm.org/D80950.

[mydeveloperday]

This is interesting.. I like it in that its a "Leave" ... but part of me would like this choice of default behind an option. As with the previous changes in this area I'm uncomfortable about us changing how it behaves, but would like the extra capability to choose.. I'm going to say Approve, but I'm interested in your opinion about if we SHOULD have an option or not?

[HazardyKnusperkeks]

I think this is a reasonable compromise.

[owenca]

This is interesting.. I like it in that its a "Leave" ... but part of me would like this choice of default behind an option. As with the previous changes in this area I'm uncomfortable about us changing how it behaves, but would like the extra capability to choose.. I'm going to say Approve, but I'm interested in your opinion about if we SHOULD have an option or not?

I don't think it warrants an option. (If an option is to be added in the future, the default should be Leave.) This is because whether to break before a << that is between two string literals depends on what the string literals are. If you have code like the following in the same directory, no option value (other than Leave) would help:

// Don't break:
QTest::newRow("test") << "" << "" << "" << "" << "" << 1 << 1 << 1 << 1 << 1;

// Break:
    OS << I->Tok->Tok.getName() << "["
       << "T=" << (unsigned)I->Tok->getType()
       << ", OC=" << I->Tok->OriginalColumn << ", \"" << I->Tok->TokenText
       << "\"] ";

Like some of the comments from https://reviews.llvm.org/D80950, I'm still of the opinion that the (undocumented) behavior of "always breaking" is a bug, even though the fix should be "leave" instead of "always merging" as done in d68826d.

[Alexolut]

Will verifyNoChange here be better suitable?

[owenca]

No, because the single-argument version of verifyFormat is a more stringent test than verifyNoChange as the former also tests the input with all optional whitespace characters between tokens removed.

[owenca]

/cherry-pick 8fe39e6

[llvmbot]

Failed to cherry-pick: 8fe39e6

https://github.com/llvm/llvm-project/actions/runs/9321579020

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[owenca]

We should backport it to 18.1.7 IMO. See #93034 and #93958. WDYT @mydeveloperday @HazardyKnusperkeks?

[mydeveloperday]

I agree

…

On Sat, 1 Jun 2024 at 04:12, Owen Pan ***@***.***> wrote: We should backport it to 18.1.7 IMO. See #93034 <#93034> and #93958 <#93958>. WDYT @mydeveloperday <https://github.com/mydeveloperday> @HazardyKnusperkeks <https://github.com/HazardyKnusperkeks>? — Reply to this email directly, view it on GitHub <#92214 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD2NSCVERVYNKSWWFZQYRV3ZFE32RAVCNFSM6AAAAABHXLU522VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGI2TSMRQGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>