[clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs #76673
Conversation
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 llvm#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 llvm#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.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-format Author: James Grant (jamesg-nz) ChangesFirstly, 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. Full diff: https://github.com/llvm/llvm-project/pull/76673.diff 2 Files Affected:
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 &State) {
const auto &CurrentState = 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;
|
|
n.b. there's relevant test coverage provided by existing |
| " });", | ||
| Style); | ||
|
|
||
| for (unsigned l : {0, 41}) { |
There was a problem hiding this comment.
Don't loop. Pick some values.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
They make it hard to see which is failing, yes. But also why introduce additional runtime for the tests, if there is no real gain?
Have one separate check for ColumnLimit = 0 case. Unroll one loop (that doesn't use 0) into two verifications.
| Current.is(TT_LambdaLBrace) && Previous.isNot(TT_LineComment)) { | ||
| auto LambdaBodyLength = getLengthToMatchingParen(Current, State.Stack); | ||
| return LambdaBodyLength > getColumnLimit(State); | ||
| if (Current.MatchingParen->MustBreakBefore) |
There was a problem hiding this comment.
I noticed there is code in TokenAnnotator than can set MustBreakBefore on the lambda left brace token:
llvm-project/clang/lib/Format/TokenAnnotator.cpp
Lines 5297 to 5299 in 0e01c72
Introduced in 37c2233 seemingly for code like this:
auto select = [this]() -> std::unique_ptr<Object> { return MyAssignment::SelectFromList(this); }Both before this current PR and here the MustBreakBefore field of the Current 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?
There was a problem hiding this comment.
I noticed there is code in
TokenAnnotatorthan can setMustBreakBeforeon the lambda left brace token:llvm-project/clang/lib/Format/TokenAnnotator.cpp
Lines 5297 to 5299 in 0e01c72
Introduced in 37c2233 seemingly for code like this:
auto select = [this]() -> std::unique_ptr<Object> { return MyAssignment::SelectFromList(this); }Both before this current PR and here the
MustBreakBeforefield of theCurrenttoken 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
TokenAnnotatorbe changed and maybe only setCanBreakBefore?
So as far as I put everything together we should break before the {, if BeforeLambdaBody is set. Except for short lambdas, if AllowShortLambdasOnASingleLine matches our lambda and it would fit on a single line. If the lambda has to be broken somewhere, it should break before {.
What is needed to be changed I don't know, but also have not the time to investigate. I'm willing to hear your analysis, or maybe @mydeveloperday can shed some light into the issue?
There was a problem hiding this comment.
@HazardyKnusperkeks - sorry late response. After looking into it, I don't believe there's an issue.
The code I was talking about above:
llvm-project/clang/lib/Format/TokenAnnotator.cpp
Lines 5297 to 5299 in 0e01c72
Results in MustBreakBefore being set for the token (the left lambda brace), which therefore results in CanBreakBefore being set. The code exists as there's conditions (e.g. Left.is(TT_PointerOrReference)) in TokenAnnotator::canBreakBefore() which return false for the left lambda brace in lambdas like these:
auto select = [this]() -> const Library::Object * { return MyAssignment::SelectFromList(this); };
auto select = [this]() -> const Library::Object & { return MyAssignment::SelectFromList(this); };
auto select = [this]() -> std::unique_ptr<Object> { return MyAssignment::SelectFromList(this); };"It's the fact it sets CanBreakBefore that matters so it enters the lambda section in ContinuationIndenter::mustBreak().
Just checking Current.MatchingParen->MustBreakBefore inside is sufficient as TokenAnnotator::mustBreakBefore() returns true when it is called for the right lambda:
llvm-project/clang/lib/Format/TokenAnnotator.cpp
Lines 5283 to 5293 in 0e01c72
Thus there is no need to look at Current.MustBreakBefore. However the generic condition that checks Current.MustBreakBefore for all tokens is in the condition for the next if() statement down, so could move the lambda if() statement down beneath that. But there's no need to do this at the moment.
So this PR looks OK as-is with respect to this.
This was fixed before 20.1.8.
If possible, please split them into two patches for easier and speedier reviews. |
|
Ping @mydeveloperday |
4 participants
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.