Skip to content

Commit 210e7b3

Browse files
jp4a50owenca
jp4a50
authored and
owenca
committed
[clang-format] Improve line-breaking in LambdaBodyIndentation: OuterScope
Avoid unnecessarily aggressive line-breaking when using "LambdaBodyIndentation: OuterScope" with argument bin-packing. Differential Revision: https://reviews.llvm.org/D148131
1 parent ec9f218 commit 210e7b3

File tree

2 files changed

+136
-64
lines changed

2 files changed

+136
-64
lines changed

‎clang/lib/Format/ContinuationIndenter.cpp

Lines changed: 77 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -318,10 +318,11 @@ bool ContinuationIndenter::canBreak(const LineState &State) {
318318
return false;
319319

320320
// Don't create a 'hanging' indent if there are multiple blocks in a single
321-
// statement.
321+
// statement and we are aligning lambda blocks to their signatures.
322322
if (Previous.is(tok::l_brace) && State.Stack.size() > 1 &&
323323
State.Stack[State.Stack.size() - 2].NestedBlockInlined &&
324-
State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks) {
324+
State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks &&
325+
Style.LambdaBodyIndentation == FormatStyle::LBI_Signature) {
325326
return false;
326327
}
327328

@@ -335,6 +336,11 @@ bool ContinuationIndenter::canBreak(const LineState &State) {
335336
// If binary operators are moved to the next line (including commas for some
336337
// styles of constructor initializers), that's always ok.
337338
if (!Current.isOneOf(TT_BinaryOperator, tok::comma) &&
339+
// Allow breaking opening brace of lambdas (when passed as function
340+
// arguments) to a new line when BeforeLambdaBody brace wrapping is
341+
// enabled.
342+
(!Style.BraceWrapping.BeforeLambdaBody ||
343+
Current.isNot(TT_LambdaLBrace)) &&
338344
CurrentState.NoLineBreakInOperand) {
339345
return false;
340346
}
@@ -662,34 +668,37 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
662668
const FormatToken &Previous = *State.NextToken->Previous;
663669
auto &CurrentState = State.Stack.back();
664670

665-
bool DisallowLineBreaksOnThisLine = Style.isCpp() && [&Current] {
666-
// Deal with lambda arguments in C++. The aim here is to ensure that we
667-
// don't over-indent lambda function bodies when lambdas are passed as
668-
// arguments to function calls. We do this by ensuring that either all
669-
// arguments (including any lambdas) go on the same line as the function
670-
// call, or we break before the first argument.
671-
auto PrevNonComment = Current.getPreviousNonComment();
672-
if (!PrevNonComment || PrevNonComment->isNot(tok::l_paren))
673-
return false;
674-
if (Current.isOneOf(tok::comment, tok::l_paren, TT_LambdaLSquare))
675-
return false;
676-
auto BlockParameterCount = PrevNonComment->BlockParameterCount;
677-
if (BlockParameterCount == 0)
678-
return false;
671+
bool DisallowLineBreaksOnThisLine =
672+
Style.LambdaBodyIndentation == FormatStyle::LBI_Signature &&
673+
Style.isCpp() && [&Current] {
674+
// Deal with lambda arguments in C++. The aim here is to ensure that we
675+
// don't over-indent lambda function bodies when lambdas are passed as
676+
// arguments to function calls. We do this by ensuring that either all
677+
// arguments (including any lambdas) go on the same line as the function
678+
// call, or we break before the first argument.
679+
auto PrevNonComment = Current.getPreviousNonComment();
680+
if (!PrevNonComment || PrevNonComment->isNot(tok::l_paren))
681+
return false;
682+
if (Current.isOneOf(tok::comment, tok::l_paren, TT_LambdaLSquare))
683+
return false;
684+
auto BlockParameterCount = PrevNonComment->BlockParameterCount;
685+
if (BlockParameterCount == 0)
686+
return false;
679687

680-
// Multiple lambdas in the same function call.
681-
if (BlockParameterCount > 1)
682-
return true;
688+
// Multiple lambdas in the same function call.
689+
if (BlockParameterCount > 1)
690+
return true;
683691

684-
// A lambda followed by another arg.
685-
if (!PrevNonComment->Role)
686-
return false;
687-
auto Comma = PrevNonComment->Role->lastComma();
688-
if (!Comma)
689-
return false;
690-
auto Next = Comma->getNextNonComment();
691-
return Next && !Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
692-
}();
692+
// A lambda followed by another arg.
693+
if (!PrevNonComment->Role)
694+
return false;
695+
auto Comma = PrevNonComment->Role->lastComma();
696+
if (!Comma)
697+
return false;
698+
auto Next = Comma->getNextNonComment();
699+
return Next &&
700+
!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
701+
}();
693702

694703
if (DisallowLineBreaksOnThisLine)
695704
State.NoLineBreak = true;
@@ -1067,9 +1076,40 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
10671076
NestedBlockSpecialCase ||
10681077
(Current.MatchingParen &&
10691078
Current.MatchingParen->is(TT_RequiresExpressionLBrace));
1070-
if (!NestedBlockSpecialCase)
1071-
for (ParenState &PState : llvm::drop_end(State.Stack))
1072-
PState.BreakBeforeParameter = true;
1079+
if (!NestedBlockSpecialCase) {
1080+
auto ParentLevelIt = std::next(State.Stack.rbegin());
1081+
if (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope &&
1082+
Current.MatchingParen && Current.MatchingParen->is(TT_LambdaLBrace)) {
1083+
// If the first character on the new line is a lambda's closing brace, the
1084+
// stack still contains that lambda's parenthesis. As such, we need to
1085+
// recurse further down the stack than usual to find the parenthesis level
1086+
// containing the lambda, which is where we want to set
1087+
// BreakBeforeParameter.
1088+
//
1089+
// We specifically special case "OuterScope"-formatted lambdas here
1090+
// because, when using that setting, breaking before the parameter
1091+
// directly following the lambda is particularly unsightly. However, when
1092+
// "OuterScope" is not set, the logic to find the parent parenthesis level
1093+
// still appears to be sometimes incorrect. It has not been fixed yet
1094+
// because it would lead to significant changes in existing behaviour.
1095+
//
1096+
// TODO: fix the non-"OuterScope" case too.
1097+
auto FindCurrentLevel = [&](const auto &It) {
1098+
return std::find_if(It, State.Stack.rend(), [](const auto &PState) {
1099+
return PState.Tok != nullptr; // Ignore fake parens.
1100+
});
1101+
};
1102+
auto MaybeIncrement = [&](const auto &It) {
1103+
return It != State.Stack.rend() ? std::next(It) : It;
1104+
};
1105+
auto LambdaLevelIt = FindCurrentLevel(State.Stack.rbegin());
1106+
auto LevelContainingLambdaIt =
1107+
FindCurrentLevel(MaybeIncrement(LambdaLevelIt));
1108+
ParentLevelIt = MaybeIncrement(LevelContainingLambdaIt);
1109+
}
1110+
for (auto I = ParentLevelIt, E = State.Stack.rend(); I != E; ++I)
1111+
I->BreakBeforeParameter = true;
1112+
}
10731113

10741114
if (PreviousNonComment &&
10751115
!PreviousNonComment->isOneOf(tok::comma, tok::colon, tok::semi) &&
@@ -1079,7 +1119,11 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
10791119
!PreviousNonComment->isOneOf(
10801120
TT_BinaryOperator, TT_FunctionAnnotationRParen, TT_JavaAnnotation,
10811121
TT_LeadingJavaAnnotation) &&
1082-
Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope()) {
1122+
Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope() &&
1123+
// We don't want to enforce line breaks for subsequent arguments just
1124+
// because we have been forced to break before a lambda body.
1125+
(!Style.BraceWrapping.BeforeLambdaBody ||
1126+
Current.isNot(TT_LambdaLBrace))) {
10831127
CurrentState.BreakBeforeParameter = true;
10841128
}
10851129

@@ -1098,7 +1142,7 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
10981142

10991143
if (CurrentState.AvoidBinPacking) {
11001144
// If we are breaking after '(', '{', '<', or this is the break after a ':'
1101-
// to start a member initializater list in a constructor, this should not
1145+
// to start a member initializer list in a constructor, this should not
11021146
// be considered bin packing unless the relevant AllowAll option is false or
11031147
// this is a dict/object literal.
11041148
bool PreviousIsBreakingCtorInitializerColon =

‎clang/unittests/Format/FormatTest.cpp

Lines changed: 59 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22524,8 +22524,7 @@ TEST_F(FormatTest, FormatsLambdas) {
2252422524
" []() -> auto {\n"
2252522525
" int b = 32;\n"
2252622526
" return 3;\n"
22527-
" },\n"
22528-
" foo, bar)\n"
22527+
" }, foo, bar)\n"
2252922528
" .foo();\n"
2253022529
"}",
2253122530
Style);
@@ -22551,61 +22550,90 @@ TEST_F(FormatTest, FormatsLambdas) {
2255122550
" })));\n"
2255222551
"}",
2255322552
Style);
22554-
Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
22555-
verifyFormat("void foo() {\n"
22556-
" aFunction(\n"
22557-
" 1, b(c(\n"
22558-
" [](d) -> Foo {\n"
22559-
" auto f = e(d);\n"
22560-
" return f;\n"
22561-
" },\n"
22562-
" foo, Bar{},\n"
22563-
" [] {\n"
22564-
" auto g = h();\n"
22565-
" return g;\n"
22566-
" },\n"
22567-
" baz)));\n"
22568-
"}",
22569-
Style);
2257022553
verifyFormat("void foo() {\n"
2257122554
" aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
22572-
" auto f = e(\n"
22573-
" foo,\n"
22574-
" [&] {\n"
22555+
" auto f = e(foo, [&] {\n"
2257522556
" auto g = h();\n"
2257622557
" return g;\n"
22577-
" },\n"
22578-
" qux,\n"
22579-
" [&] -> Bar {\n"
22558+
" }, qux, [&] -> Bar {\n"
2258022559
" auto i = j();\n"
2258122560
" return i;\n"
2258222561
" });\n"
2258322562
" return f;\n"
2258422563
" })));\n"
2258522564
"}",
2258622565
Style);
22587-
verifyFormat("Namespace::Foo::Foo(\n"
22588-
" LongClassName bar, AnotherLongClassName baz)\n"
22566+
verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
22567+
" AnotherLongClassName baz)\n"
2258922568
" : baz{baz}, func{[&] {\n"
2259022569
" auto qux = bar;\n"
2259122570
" return aFunkyFunctionCall(qux);\n"
2259222571
"}} {}",
2259322572
Style);
22573+
Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
22574+
// FIXME: The following test should pass, but fails at the time of writing.
22575+
#if 0
22576+
// As long as all the non-lambda arguments fit on a single line, AlwaysBreak
22577+
// doesn't force an initial line break, even if lambdas span multiple lines.
22578+
verifyFormat("void foo() {\n"
22579+
" aFunction(\n"
22580+
" [](d) -> Foo {\n"
22581+
" auto f = e(d);\n"
22582+
" return f;\n"
22583+
" }, foo, Bar{}, [] {\n"
22584+
" auto g = h();\n"
22585+
" return g;\n"
22586+
" }, baz);\n"
22587+
"}",
22588+
Style);
22589+
#endif
22590+
// A long non-lambda argument forces arguments to span multiple lines and thus
22591+
// forces an initial line break when using AlwaysBreak.
22592+
verifyFormat("void foo() {\n"
22593+
" aFunction(\n"
22594+
" 1,\n"
22595+
" [](d) -> Foo {\n"
22596+
" auto f = e(d);\n"
22597+
" return f;\n"
22598+
" }, foo, Bar{},\n"
22599+
" [] {\n"
22600+
" auto g = h();\n"
22601+
" return g;\n"
22602+
" }, bazzzzz,\n"
22603+
" quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
22604+
"}",
22605+
Style);
22606+
Style.BinPackArguments = false;
22607+
verifyFormat("void foo() {\n"
22608+
" aFunction(\n"
22609+
" 1,\n"
22610+
" [](d) -> Foo {\n"
22611+
" auto f = e(d);\n"
22612+
" return f;\n"
22613+
" },\n"
22614+
" foo,\n"
22615+
" Bar{},\n"
22616+
" [] {\n"
22617+
" auto g = h();\n"
22618+
" return g;\n"
22619+
" },\n"
22620+
" bazzzzz,\n"
22621+
" quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
22622+
"}",
22623+
Style);
22624+
Style.BinPackArguments = true;
2259422625
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
2259522626
Style.BraceWrapping.BeforeLambdaBody = true;
2259622627
verifyFormat("void foo() {\n"
2259722628
" aFunction(\n"
22598-
" 1, b(c(foo, Bar{}, baz,\n"
22599-
" [](d) -> Foo\n"
22629+
" 1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
2260022630
" {\n"
2260122631
" auto f = e(\n"
2260222632
" [&]\n"
2260322633
" {\n"
2260422634
" auto g = h();\n"
2260522635
" return g;\n"
22606-
" },\n"
22607-
" qux,\n"
22608-
" [&] -> Bar\n"
22636+
" }, qux, [&] -> Bar\n"
2260922637
" {\n"
2261022638
" auto i = j();\n"
2261122639
" return i;\n"

0 commit comments

Comments
 (0)