[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
https://github.com/jamesg-nz edited https://github.com/llvm/llvm-project/pull/76673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
@@ -366,8 +366,14 @@ bool ContinuationIndenter::mustBreak(const LineState
) {
const auto = State.Stack.back();
if (Style.BraceWrapping.BeforeLambdaBody && Current.CanBreakBefore &&
Current.is(TT_LambdaLBrace) && Previous.isNot(TT_LineComment)) {
-auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
-return LambdaBodyLength > getColumnLimit(State);
+if (Current.MatchingParen->MustBreakBefore)
jamesg-nz wrote:
I noticed there is code in `TokenAnnotator` than can set `MustBreakBefore` on
the lambda left brace token:
https://github.com/llvm/llvm-project/blob/0e01c72c5645259d9a08a1a7ed39cb5cc41ce311/clang/lib/Format/TokenAnnotator.cpp#L5297-L5299
Introduced in 37c2233097ac44697b87228d86eef1fce10ea5c1 seemingly for code like
this:
```cpp
auto select = [this]() -> std::unique_ptr { return
MyAssignment::SelectFromList(this); }
```
Both before this current PR and here the `MustBreakBefore` state of the token
is ignored. Should behavior change to observe this and thus force a break?
Would seem the _BraceWrapping.BeforeLambdaBody_ setting would then force lambda
across multiple lines if on, but not if off.
Should the code in `TokenAnnotator` be changed and maybe only set
`CanBreakBefore`?
https://github.com/llvm/llvm-project/pull/76673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
jamesg-nz wrote: > I think that's not the right way to fix the issue. > > Why are the 2 lines formatted differently? It seems to me that this fixes the > symptom, not the cause. Because for the line where brackets are used it meets the condition in the next `if` statement down: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L122 ... and a penalty of zero is returned. Similar exclusion exists for precomputing the column formats: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L191 But this ultimately ends up simply as performance optimization as they're never considered later in any case. Whereas if the braces are used instead of brackets it does not meet the _not a left brace_ condition, and gets to the severe penalty return: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L140 . This is because suitable column formats are not found, as there was less than 5 commas found in the list when precomputing the column formats, and so none are generated: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L273 Seems a little odd to return severe penalty if no column formats are available as opposed to having them, but none fit right now. But I'm thinking maybe better to somehow add a condition that if the left brace appears within a short lambda body that'll end up on a single line then a penalty of zero is also returned. More/better targeted change. Ultimately I believe comes down to finding a good justification to return zero penalty when the braces are used inside lambda bodies. Maybe affects other _short_ functions too like inlines - need to check. https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
@@ -22965,6 +22965,84 @@ TEST_F(FormatTest, EmptyLinesInLambdas) {
"};");
}
+TEST_F(FormatTest, BreakBeforeLambdaBodyWrapping) {
+ verifyFormat("connect([]() {\n"
+ " foo();\n"
+ " bar();\n"
+ "});");
+
+ auto Style = getLLVMStyle();
+ Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+ Style.BraceWrapping.BeforeLambdaBody = true;
+
+ verifyFormat("connect(\n"
+ "[]()\n"
+ "{\n"
+ " foo();\n"
+ " bar();\n"
+ "});",
+ Style);
+
+ for (unsigned l : {0, 41}) {
jamesg-nz wrote:
Now testing ColumnLimit = 0 once separately. Unrolled remaining loop (that
doesn't test = 0) into two verifications.
Loops make it hard to see in test results for what value it is failing for, I
guess. Although saw other people had used them in that file.
https://github.com/llvm/llvm-project/pull/76673
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
https://github.com/jamesg-nz updated
https://github.com/llvm/llvm-project/pull/76673
>From 04885844162b5390d8041a44a1895ad6ac160228 Mon Sep 17 00:00:00 2001
From: James Grant <42079499+jamesg...@users.noreply.github.com>
Date: Mon, 1 Jan 2024 20:27:41 +1300
Subject: [PATCH 1/2] [clang-format] Fix erroneous
BraceWrapping.BeforeLambdaBody column calcs
Firstly, must check ColumnLimit > 0 before comparing calculated columns
to the limit. Otherwise it always breaks before the brace if ColumnLimit
= 0 (no limit). Fixes #50275
Secondly, the lambda body length alone is currently compared with the
column limit. Should instead be comparing a column position, which
includes everything before the lambda body, the body length itself, and
any unbreakable tail. Fixes #59724
Thirdly, if must break before the lambda right brace, e.g. line comment
in body, then must also break before the left brace. Can't use column
calculation in this instance.
---
clang/lib/Format/ContinuationIndenter.cpp | 10 ++-
clang/unittests/Format/FormatTest.cpp | 78 +++
2 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp
b/clang/lib/Format/ContinuationIndenter.cpp
index 102504182c4505..f4f8b694f7ff51 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -366,8 +366,14 @@ bool ContinuationIndenter::mustBreak(const LineState
) {
const auto = State.Stack.back();
if (Style.BraceWrapping.BeforeLambdaBody && Current.CanBreakBefore &&
Current.is(TT_LambdaLBrace) && Previous.isNot(TT_LineComment)) {
-auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
-return LambdaBodyLength > getColumnLimit(State);
+if (Current.MatchingParen->MustBreakBefore)
+ return true;
+
+auto LambdaEnd = getLengthToMatchingParen(Current, State.Stack) +
+ Current.MatchingParen->UnbreakableTailLength +
+ State.Column - 1;
+
+return Style.ColumnLimit > 0 && LambdaEnd > getColumnLimit(State);
}
if (Current.MustBreakBefore ||
(Current.is(TT_InlineASMColon) &&
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..f5aadec3500ccb 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -22965,6 +22965,84 @@ TEST_F(FormatTest, EmptyLinesInLambdas) {
"};");
}
+TEST_F(FormatTest, BreakBeforeLambdaBodyWrapping) {
+ verifyFormat("connect([]() {\n"
+ " foo();\n"
+ " bar();\n"
+ "});");
+
+ auto Style = getLLVMStyle();
+ Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+ Style.BraceWrapping.BeforeLambdaBody = true;
+
+ verifyFormat("connect(\n"
+ "[]()\n"
+ "{\n"
+ " foo();\n"
+ " bar();\n"
+ "});",
+ Style);
+
+ for (unsigned l : {0, 41}) {
+Style.ColumnLimit = l;
+verifyFormat("auto lambda = []() { return foo + bar; };", Style);
+ }
+ for (unsigned l : {40, 22}) {
+Style.ColumnLimit = l;
+verifyFormat("auto lambda = []()\n"
+ "{ return foo + bar; };",
+ Style);
+ }
+ Style.ColumnLimit = 21;
+ verifyFormat("auto lambda = []()\n"
+ "{\n"
+ " return foo + bar;\n"
+ "};",
+ Style);
+
+ for (unsigned l : {0, 67}) {
+Style.ColumnLimit = l;
+verifyFormat(
+"auto result = [](int foo, int bar) { return foo + bar; }(foo, bar);",
+Style);
+ }
+ Style.ColumnLimit = 66;
+ verifyFormat("auto result = [](int foo, int bar)\n"
+ "{ return foo + bar; }(foo, bar);",
+ Style);
+
+ Style.ColumnLimit = 36;
+ verifyFormat("myFunc([&]() { return foo + bar; });", Style);
+ Style.ColumnLimit = 35;
+ verifyFormat("myFunc([&]()\n"
+ " { return foo + bar; });",
+ Style);
+
+ Style = getGoogleStyleWithColumns(100);
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ Style.IndentWidth = 4;
+ verifyFormat(
+ "void Func()\n"
+ "{\n"
+ "[]()\n"
+ "{\n"
+ "return TestVeryLongThingName::TestVeryLongFunctionName()\n"
+ ".TestAnotherVeryVeryLongFunctionName();\n"
+ "}\n"
+ "}\n",
+ Style);
+ verifyFormat(
+ "void Func()\n"
+ "{\n"
+ "[]()\n"
+ "{\n"
+ "return TestVeryLongThingName::TestVeryLongFunctionName()\n"
+ ".TestAnotherVeryVeryVeryLongFunctionName();\n"
+ "}\n"
+ "}\n",
+ Style);
+}
+
TEST_F(FormatTest, FormatsBlocks) {
FormatStyle ShortBlocks = getLLVMStyle();
ShortBlocks.AllowShortBlocksOnASingleLine = FormatStyle::SBS_Always;
>From 346e7da1f6eb184f7fc5212af94f5e7c83d5a494 Mon Sep 17 00:00:00 2001
From: James Grant
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
jamesg-nz wrote: Running debug build clang-format with `-debug` parameter against code provided in #56350 (and its duplicate #58469) you see `Penalty for line: 1`. So pretty obvious the [severe penalty](https://github.com/llvm/llvm-project/blob/703e83611cd8bb7174ae76ba2e301f5a5e88b905/clang/lib/Format/FormatToken.cpp#L140) is responsible. Thoughts on this change? Is the removal of the severe penalty being applied too broadly? ... or is it correct/OK? https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
https://github.com/jamesg-nz created
https://github.com/llvm/llvm-project/pull/76675
If there are possible column formats, but they weren't selected because they
don't fit within remaining characters for the current path then applying severe
penalty to induce column layout by selection of a different path seems fair.
But if due to style configuration or what the input code is, there are no
possible column formats, different paths aren't going to have column layouts.
Seems wrong to apply the severe penalty to induce column layouts if there are
none available.
It just causes selection of sub-optimal paths, e.g. get bad formatting when
brace initializers are used inside lambda bodies.
Fixes #56350
>From 68f7dee10e20b4ceeaba67f1ebe6b1153647e6ff Mon Sep 17 00:00:00 2001
From: James Grant <42079499+jamesg...@users.noreply.github.com>
Date: Mon, 1 Jan 2024 21:35:11 +1300
Subject: [PATCH] [clang-format] Don't apply severe penalty if no possible
column formats
If there are possible column formats, but they weren't selected because
they don't fit within remaining characters for the current path then
applying severe penalty to induce column layout by selection of a
different path seems fair.
But if due to style configuration or what the input code is, there are
no possible column formats, different paths aren't going to have column
layouts. Seems wrong to apply the severe penalty to induce column
layouts if there are none available.
It just causes selection of sub-optimal paths, e.g. get bad formatting
when brace initializers are used inside lambda bodies.
Fixes #56350
---
clang/lib/Format/FormatToken.cpp | 4 ++--
clang/unittests/Format/FormatTest.cpp | 15 +++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 7a2df8c53952f9..b791c5a26bbe3a 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -113,8 +113,8 @@ unsigned CommaSeparatedList::formatAfterToken(LineState
,
if (!State.NextToken || !State.NextToken->Previous)
return 0;
- if (Formats.size() == 1)
-return 0; // Handled by formatFromToken
+ if (Formats.size() <= 1)
+return 0; // Handled by formatFromToken (1) or avoid severe penalty (0).
// Ensure that we start on the opening brace.
const FormatToken *LBrace =
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..b7350b2fe66d9b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13875,6 +13875,21 @@ TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
getLLVMStyleWithColumns(35));
verifyFormat("aa(aaa, {},\n"
" aaa);");
+
+ // No possible column formats, don't want the optimal paths penalized.
+ verifyFormat(
+ "waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10} * w::m; }};");
+ verifyFormat("SomeType something1([](const Input ) -> Output { return "
+ "Output{1, 2}; },\n"
+ "[](const Input ) -> Output { return "
+ "Output{1, 2}; });");
+ FormatStyle NoBinPacking = getLLVMStyle();
+ NoBinPacking.BinPackParameters = false;
+ verifyFormat("waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10, 1, 1, "
+ "1, 1} * w::m; }};",
+ NoBinPacking);
}
TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
jamesg-nz wrote: n.b. there's relevant test coverage provided by existing `LambdaWithLineComments` test. https://github.com/llvm/llvm-project/pull/76673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs (PR #76673)
https://github.com/jamesg-nz created
https://github.com/llvm/llvm-project/pull/76673
Firstly, must check ColumnLimit > 0 before comparing calculated columns to the
limit. Otherwise it always breaks before the brace if ColumnLimit = 0 (no
limit). Fixes #50275
Secondly, the lambda body length alone is currently compared with the column
limit. Should instead be comparing a column position, which includes everything
before the lambda body, the body length itself, and any unbreakable tail. Fixes
#59724
Thirdly, if must break before the lambda right brace, e.g. line comment in
body, then must also break before the left brace. Can't use column calculation
in this instance.
>From 04885844162b5390d8041a44a1895ad6ac160228 Mon Sep 17 00:00:00 2001
From: James Grant <42079499+jamesg...@users.noreply.github.com>
Date: Mon, 1 Jan 2024 20:27:41 +1300
Subject: [PATCH] [clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody
column calcs
Firstly, must check ColumnLimit > 0 before comparing calculated columns
to the limit. Otherwise it always breaks before the brace if ColumnLimit
= 0 (no limit). Fixes #50275
Secondly, the lambda body length alone is currently compared with the
column limit. Should instead be comparing a column position, which
includes everything before the lambda body, the body length itself, and
any unbreakable tail. Fixes #59724
Thirdly, if must break before the lambda right brace, e.g. line comment
in body, then must also break before the left brace. Can't use column
calculation in this instance.
---
clang/lib/Format/ContinuationIndenter.cpp | 10 ++-
clang/unittests/Format/FormatTest.cpp | 78 +++
2 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp
b/clang/lib/Format/ContinuationIndenter.cpp
index 102504182c4505..f4f8b694f7ff51 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -366,8 +366,14 @@ bool ContinuationIndenter::mustBreak(const LineState
) {
const auto = State.Stack.back();
if (Style.BraceWrapping.BeforeLambdaBody && Current.CanBreakBefore &&
Current.is(TT_LambdaLBrace) && Previous.isNot(TT_LineComment)) {
-auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack);
-return LambdaBodyLength > getColumnLimit(State);
+if (Current.MatchingParen->MustBreakBefore)
+ return true;
+
+auto LambdaEnd = getLengthToMatchingParen(Current, State.Stack) +
+ Current.MatchingParen->UnbreakableTailLength +
+ State.Column - 1;
+
+return Style.ColumnLimit > 0 && LambdaEnd > getColumnLimit(State);
}
if (Current.MustBreakBefore ||
(Current.is(TT_InlineASMColon) &&
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..f5aadec3500ccb 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -22965,6 +22965,84 @@ TEST_F(FormatTest, EmptyLinesInLambdas) {
"};");
}
+TEST_F(FormatTest, BreakBeforeLambdaBodyWrapping) {
+ verifyFormat("connect([]() {\n"
+ " foo();\n"
+ " bar();\n"
+ "});");
+
+ auto Style = getLLVMStyle();
+ Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+ Style.BraceWrapping.BeforeLambdaBody = true;
+
+ verifyFormat("connect(\n"
+ "[]()\n"
+ "{\n"
+ " foo();\n"
+ " bar();\n"
+ "});",
+ Style);
+
+ for (unsigned l : {0, 41}) {
+Style.ColumnLimit = l;
+verifyFormat("auto lambda = []() { return foo + bar; };", Style);
+ }
+ for (unsigned l : {40, 22}) {
+Style.ColumnLimit = l;
+verifyFormat("auto lambda = []()\n"
+ "{ return foo + bar; };",
+ Style);
+ }
+ Style.ColumnLimit = 21;
+ verifyFormat("auto lambda = []()\n"
+ "{\n"
+ " return foo + bar;\n"
+ "};",
+ Style);
+
+ for (unsigned l : {0, 67}) {
+Style.ColumnLimit = l;
+verifyFormat(
+"auto result = [](int foo, int bar) { return foo + bar; }(foo, bar);",
+Style);
+ }
+ Style.ColumnLimit = 66;
+ verifyFormat("auto result = [](int foo, int bar)\n"
+ "{ return foo + bar; }(foo, bar);",
+ Style);
+
+ Style.ColumnLimit = 36;
+ verifyFormat("myFunc([&]() { return foo + bar; });", Style);
+ Style.ColumnLimit = 35;
+ verifyFormat("myFunc([&]()\n"
+ " { return foo + bar; });",
+ Style);
+
+ Style = getGoogleStyleWithColumns(100);
+ Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+ Style.IndentWidth = 4;
+ verifyFormat(
+ "void Func()\n"
+ "{\n"
+ "[]()\n"
+ "{\n"
+ "return TestVeryLongThingName::TestVeryLongFunctionName()\n"
+ ".TestAnotherVeryVeryLongFunctionName();\n"
