Skip to content

[clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs #76673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs #76673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0488584
[clang-format] Fix erroneous BraceWrapping.BeforeLambdaBody column calcs
jamesg-nz Jan 1, 2024
346e7da
Per feedback, remove loops from test.
jamesg-nz Jan 1, 2024
4f35985
Merge branch 'main' into jg-beforelambdabody-calc-fixes
jamesg-nz Jan 29, 2025
6c55959
Merge branch 'main' into jg-beforelambdabody-calc-fixes
jamesg-nz Jul 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions clang/lib/Format/ContinuationIndenter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@jamesg-nz jamesg-nz Jan 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
(Left.isPointerOrReference() || Left.is(TT_TemplateCloser))) {
return true;

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?

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
(Left.isPointerOrReference() || Left.is(TT_TemplateCloser))) {
return true;

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?

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?

Copy link
Contributor Author

@jamesg-nz jamesg-nz Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) &&
(Left.isPointerOrReference() || Left.is(TT_TemplateCloser))) {
return true;

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

if (Left.is(TT_LambdaLBrace)) {
if (IsFunctionArgument(Left) &&
Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Inline) {
return false;
}
if (Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_None ||
Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Inline ||
(!Left.Children.empty() &&
Style.AllowShortLambdasOnASingleLine == FormatStyle::SLS_Empty)) {
return true;

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.

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) &&
Expand Down
78 changes: 78 additions & 0 deletions clang/unittests/Format/FormatTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}) {
Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't loop. Pick some values.

Copy link
Contributor Author

@jamesg-nz jamesg-nz Jan 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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;
Expand Down