[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[via cfe-commits]

mydeveloperday wrote:

I agree


On Sat, 1 Jun 2024 at 04:12, Owen Pan ***@***.***> wrote:

> We should backport it to 18.1.7 IMO. See #93034
>  and #93958
> . WDYT @mydeveloperday
>  @HazardyKnusperkeks
> ?
>
> —
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>


https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

owenca wrote:

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

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[via cfe-commits]

llvmbot wrote:


Failed to cherry-pick: 8fe39e64c0ef0a1aefce3c1187c5822343caeedd

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](https://github.com/llvm/llvm-project/compare)

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

owenca wrote:

/cherry-pick 
https://github.com/llvm/llvm-project/commit/8fe39e64c0ef0a1aefce3c1187c5822343caeedd

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

https://github.com/owenca milestoned 
https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

@@ -10539,6 +10539,17 @@ TEST_F(FormatTest, KeepStringLabelValuePairsOnALine) {
   "  bbb);");
 }
 
+TEST_F(FormatTest, WrapBeforeInsertionOperatorbetweenStringLiterals) {
+  verifyFormat("QStringList() << \"foo\" << \"bar\";");

owenca wrote:

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.

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[via cfe-commits]

@@ -10539,6 +10539,17 @@ TEST_F(FormatTest, KeepStringLabelValuePairsOnALine) {
   "  bbb);");
 }
 
+TEST_F(FormatTest, WrapBeforeInsertionOperatorbetweenStringLiterals) {
+  verifyFormat("QStringList() << \"foo\" << \"bar\";");

Alexolut wrote:

Will `verifyNoChange` here be better suitable?

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

https://github.com/owenca closed https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

owenca wrote:

> 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 
d68826dfbd98.

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Björn Schäpers via cfe-commits]

https://github.com/HazardyKnusperkeks approved this pull request.

I think this is a reasonable compromise.

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[via cfe-commits]

https://github.com/mydeveloperday approved this pull request.

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?

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

owenca wrote:

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

https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[via cfe-commits]

llvmbot wrote:




@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
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 ,
 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) {
   "  bbb);");
 }
 
+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(
   "a =\n"

``




https://github.com/llvm/llvm-project/pull/92214
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Don't always break before << between string literals (PR #92214)

[Owen Pan via cfe-commits]

https://github.com/owenca created 
https://github.com/llvm/llvm-project/pull/92214

Instead, leave the line wrapping as is.

Fixes #43887.
Fixes #44363.

>From 40eeb958cef55465fdcee66ab385928c1f202e50 Mon Sep 17 00:00:00 2001
From: Owen Pan 
Date: Tue, 14 May 2024 19:14:15 -0700
Subject: [PATCH] [clang-format] Don't always break before << between string
 literals

Instead, leave the line wrapping as is.

Fixes #43887.
Fixes #44363.
---
 clang/lib/Format/TokenAnnotator.cpp   |  7 +--
 clang/unittests/Format/FormatTest.cpp | 11 +++
 2 files changed, 16 insertions(+), 2 deletions(-)

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 ,
 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) {
   "  bbb);");
 }
 
+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(
   "a =\n"

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits