[clang] 210e7b3 - [clang-format] Improve line-breaking in LambdaBodyIndentation: OuterScope
Owen Pan via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 8 14:34:08 PDT 2023
Author: Jon Phillips
Date: 2023-09-08T14:34:00-07:00
New Revision: 210e7b3ca773c07c2a8c2fd8f471db5c6724f3bc
URL: https://github.com/llvm/llvm-project/commit/210e7b3ca773c07c2a8c2fd8f471db5c6724f3bc
DIFF: https://github.com/llvm/llvm-project/commit/210e7b3ca773c07c2a8c2fd8f471db5c6724f3bc.diff
LOG: [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
Added:
Modified:
clang/lib/Format/ContinuationIndenter.cpp
clang/unittests/Format/FormatTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 3f15faf979e3b6e..ac62dab1b07cdca 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -318,10 +318,11 @@ bool ContinuationIndenter::canBreak(const LineState &State) {
return false;
// Don't create a 'hanging' indent if there are multiple blocks in a single
- // statement.
+ // statement and we are aligning lambda blocks to their signatures.
if (Previous.is(tok::l_brace) && State.Stack.size() > 1 &&
State.Stack[State.Stack.size() - 2].NestedBlockInlined &&
- State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks) {
+ State.Stack[State.Stack.size() - 2].HasMultipleNestedBlocks &&
+ Style.LambdaBodyIndentation == FormatStyle::LBI_Signature) {
return false;
}
@@ -335,6 +336,11 @@ bool ContinuationIndenter::canBreak(const LineState &State) {
// If binary operators are moved to the next line (including commas for some
// styles of constructor initializers), that's always ok.
if (!Current.isOneOf(TT_BinaryOperator, tok::comma) &&
+ // Allow breaking opening brace of lambdas (when passed as function
+ // arguments) to a new line when BeforeLambdaBody brace wrapping is
+ // enabled.
+ (!Style.BraceWrapping.BeforeLambdaBody ||
+ Current.isNot(TT_LambdaLBrace)) &&
CurrentState.NoLineBreakInOperand) {
return false;
}
@@ -662,34 +668,37 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
const FormatToken &Previous = *State.NextToken->Previous;
auto &CurrentState = State.Stack.back();
- bool DisallowLineBreaksOnThisLine = Style.isCpp() && [&Current] {
- // Deal with lambda arguments in C++. The aim here is to ensure that we
- // don't over-indent lambda function bodies when lambdas are passed as
- // arguments to function calls. We do this by ensuring that either all
- // arguments (including any lambdas) go on the same line as the function
- // call, or we break before the first argument.
- auto PrevNonComment = Current.getPreviousNonComment();
- if (!PrevNonComment || PrevNonComment->isNot(tok::l_paren))
- return false;
- if (Current.isOneOf(tok::comment, tok::l_paren, TT_LambdaLSquare))
- return false;
- auto BlockParameterCount = PrevNonComment->BlockParameterCount;
- if (BlockParameterCount == 0)
- return false;
+ bool DisallowLineBreaksOnThisLine =
+ Style.LambdaBodyIndentation == FormatStyle::LBI_Signature &&
+ Style.isCpp() && [&Current] {
+ // Deal with lambda arguments in C++. The aim here is to ensure that we
+ // don't over-indent lambda function bodies when lambdas are passed as
+ // arguments to function calls. We do this by ensuring that either all
+ // arguments (including any lambdas) go on the same line as the function
+ // call, or we break before the first argument.
+ auto PrevNonComment = Current.getPreviousNonComment();
+ if (!PrevNonComment || PrevNonComment->isNot(tok::l_paren))
+ return false;
+ if (Current.isOneOf(tok::comment, tok::l_paren, TT_LambdaLSquare))
+ return false;
+ auto BlockParameterCount = PrevNonComment->BlockParameterCount;
+ if (BlockParameterCount == 0)
+ return false;
- // Multiple lambdas in the same function call.
- if (BlockParameterCount > 1)
- return true;
+ // Multiple lambdas in the same function call.
+ if (BlockParameterCount > 1)
+ return true;
- // A lambda followed by another arg.
- if (!PrevNonComment->Role)
- return false;
- auto Comma = PrevNonComment->Role->lastComma();
- if (!Comma)
- return false;
- auto Next = Comma->getNextNonComment();
- return Next && !Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
- }();
+ // A lambda followed by another arg.
+ if (!PrevNonComment->Role)
+ return false;
+ auto Comma = PrevNonComment->Role->lastComma();
+ if (!Comma)
+ return false;
+ auto Next = Comma->getNextNonComment();
+ return Next &&
+ !Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret);
+ }();
if (DisallowLineBreaksOnThisLine)
State.NoLineBreak = true;
@@ -1067,9 +1076,40 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
NestedBlockSpecialCase ||
(Current.MatchingParen &&
Current.MatchingParen->is(TT_RequiresExpressionLBrace));
- if (!NestedBlockSpecialCase)
- for (ParenState &PState : llvm::drop_end(State.Stack))
- PState.BreakBeforeParameter = true;
+ if (!NestedBlockSpecialCase) {
+ auto ParentLevelIt = std::next(State.Stack.rbegin());
+ if (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope &&
+ Current.MatchingParen && Current.MatchingParen->is(TT_LambdaLBrace)) {
+ // If the first character on the new line is a lambda's closing brace, the
+ // stack still contains that lambda's parenthesis. As such, we need to
+ // recurse further down the stack than usual to find the parenthesis level
+ // containing the lambda, which is where we want to set
+ // BreakBeforeParameter.
+ //
+ // We specifically special case "OuterScope"-formatted lambdas here
+ // because, when using that setting, breaking before the parameter
+ // directly following the lambda is particularly unsightly. However, when
+ // "OuterScope" is not set, the logic to find the parent parenthesis level
+ // still appears to be sometimes incorrect. It has not been fixed yet
+ // because it would lead to significant changes in existing behaviour.
+ //
+ // TODO: fix the non-"OuterScope" case too.
+ auto FindCurrentLevel = [&](const auto &It) {
+ return std::find_if(It, State.Stack.rend(), [](const auto &PState) {
+ return PState.Tok != nullptr; // Ignore fake parens.
+ });
+ };
+ auto MaybeIncrement = [&](const auto &It) {
+ return It != State.Stack.rend() ? std::next(It) : It;
+ };
+ auto LambdaLevelIt = FindCurrentLevel(State.Stack.rbegin());
+ auto LevelContainingLambdaIt =
+ FindCurrentLevel(MaybeIncrement(LambdaLevelIt));
+ ParentLevelIt = MaybeIncrement(LevelContainingLambdaIt);
+ }
+ for (auto I = ParentLevelIt, E = State.Stack.rend(); I != E; ++I)
+ I->BreakBeforeParameter = true;
+ }
if (PreviousNonComment &&
!PreviousNonComment->isOneOf(tok::comma, tok::colon, tok::semi) &&
@@ -1079,7 +1119,11 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
!PreviousNonComment->isOneOf(
TT_BinaryOperator, TT_FunctionAnnotationRParen, TT_JavaAnnotation,
TT_LeadingJavaAnnotation) &&
- Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope()) {
+ Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope() &&
+ // We don't want to enforce line breaks for subsequent arguments just
+ // because we have been forced to break before a lambda body.
+ (!Style.BraceWrapping.BeforeLambdaBody ||
+ Current.isNot(TT_LambdaLBrace))) {
CurrentState.BreakBeforeParameter = true;
}
@@ -1098,7 +1142,7 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
if (CurrentState.AvoidBinPacking) {
// If we are breaking after '(', '{', '<', or this is the break after a ':'
- // to start a member initializater list in a constructor, this should not
+ // to start a member initializer list in a constructor, this should not
// be considered bin packing unless the relevant AllowAll option is false or
// this is a dict/object literal.
bool PreviousIsBreakingCtorInitializerColon =
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 721cf29e0db474f..cb4703804ebb36e 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -22524,8 +22524,7 @@ TEST_F(FormatTest, FormatsLambdas) {
" []() -> auto {\n"
" int b = 32;\n"
" return 3;\n"
- " },\n"
- " foo, bar)\n"
+ " }, foo, bar)\n"
" .foo();\n"
"}",
Style);
@@ -22551,32 +22550,12 @@ TEST_F(FormatTest, FormatsLambdas) {
" })));\n"
"}",
Style);
- Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
- verifyFormat("void foo() {\n"
- " aFunction(\n"
- " 1, b(c(\n"
- " [](d) -> Foo {\n"
- " auto f = e(d);\n"
- " return f;\n"
- " },\n"
- " foo, Bar{},\n"
- " [] {\n"
- " auto g = h();\n"
- " return g;\n"
- " },\n"
- " baz)));\n"
- "}",
- Style);
verifyFormat("void foo() {\n"
" aFunction(1, b(c(foo, Bar{}, baz, [](d) -> Foo {\n"
- " auto f = e(\n"
- " foo,\n"
- " [&] {\n"
+ " auto f = e(foo, [&] {\n"
" auto g = h();\n"
" return g;\n"
- " },\n"
- " qux,\n"
- " [&] -> Bar {\n"
+ " }, qux, [&] -> Bar {\n"
" auto i = j();\n"
" return i;\n"
" });\n"
@@ -22584,28 +22563,77 @@ TEST_F(FormatTest, FormatsLambdas) {
" })));\n"
"}",
Style);
- verifyFormat("Namespace::Foo::Foo(\n"
- " LongClassName bar, AnotherLongClassName baz)\n"
+ verifyFormat("Namespace::Foo::Foo(LongClassName bar,\n"
+ " AnotherLongClassName baz)\n"
" : baz{baz}, func{[&] {\n"
" auto qux = bar;\n"
" return aFunkyFunctionCall(qux);\n"
"}} {}",
Style);
+ Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+ // FIXME: The following test should pass, but fails at the time of writing.
+#if 0
+ // As long as all the non-lambda arguments fit on a single line, AlwaysBreak
+ // doesn't force an initial line break, even if lambdas span multiple lines.
+ verifyFormat("void foo() {\n"
+ " aFunction(\n"
+ " [](d) -> Foo {\n"
+ " auto f = e(d);\n"
+ " return f;\n"
+ " }, foo, Bar{}, [] {\n"
+ " auto g = h();\n"
+ " return g;\n"
+ " }, baz);\n"
+ "}",
+ Style);
+#endif
+ // A long non-lambda argument forces arguments to span multiple lines and thus
+ // forces an initial line break when using AlwaysBreak.
+ verifyFormat("void foo() {\n"
+ " aFunction(\n"
+ " 1,\n"
+ " [](d) -> Foo {\n"
+ " auto f = e(d);\n"
+ " return f;\n"
+ " }, foo, Bar{},\n"
+ " [] {\n"
+ " auto g = h();\n"
+ " return g;\n"
+ " }, bazzzzz,\n"
+ " quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
+ "}",
+ Style);
+ Style.BinPackArguments = false;
+ verifyFormat("void foo() {\n"
+ " aFunction(\n"
+ " 1,\n"
+ " [](d) -> Foo {\n"
+ " auto f = e(d);\n"
+ " return f;\n"
+ " },\n"
+ " foo,\n"
+ " Bar{},\n"
+ " [] {\n"
+ " auto g = h();\n"
+ " return g;\n"
+ " },\n"
+ " bazzzzz,\n"
+ " quuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuux);\n"
+ "}",
+ Style);
+ Style.BinPackArguments = true;
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.BeforeLambdaBody = true;
verifyFormat("void foo() {\n"
" aFunction(\n"
- " 1, b(c(foo, Bar{}, baz,\n"
- " [](d) -> Foo\n"
+ " 1, b(c(foo, Bar{}, baz, [](d) -> Foo\n"
" {\n"
" auto f = e(\n"
" [&]\n"
" {\n"
" auto g = h();\n"
" return g;\n"
- " },\n"
- " qux,\n"
- " [&] -> Bar\n"
+ " }, qux, [&] -> Bar\n"
" {\n"
" auto i = j();\n"
" return i;\n"
More information about the cfe-commits
mailing list