diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -166,6 +166,8 @@ ------------ - Fix a bug that erroneously placed function arguments on a new line despite all arguments being able to fit on the same line. +- Avoid unnecessarily aggressive line-breaking when using + ``LambdaBodyIndentation: OuterScope`` with argument bin-packing. libclang -------- diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -310,10 +310,11 @@ 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; } @@ -327,6 +328,10 @@ // 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. + !(Current.is(TT_LambdaLBrace) && Style.BraceWrapping.BeforeLambdaBody) && CurrentState.NoLineBreakInOperand) { return false; } @@ -654,8 +659,10 @@ const FormatToken &Previous = *State.NextToken->Previous; auto &CurrentState = State.Stack.back(); - bool DisallowLineBreaksOnThisLine = (Style.Language == FormatStyle::LK_Cpp || - Style.Language == FormatStyle::LK_ObjC) && [&Current] { + bool DisallowLineBreaksOnThisLine = + (Style.LambdaBodyIndentation == FormatStyle::LBI_Signature && + (Style.Language == FormatStyle::LK_Cpp || + Style.Language == FormatStyle::LK_ObjC)) && [&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 @@ -1066,9 +1073,41 @@ 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 = State.Stack.rbegin() + 1; + 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() ? (It + 1) : It; + }; + auto LambdaLevelIt = FindCurrentLevel(State.Stack.rbegin()); + auto LevelContainingLambdaIt = + FindCurrentLevel(MaybeIncrement(LambdaLevelIt)); + ParentLevelIt = MaybeIncrement(LevelContainingLambdaIt); + } + if (ParentLevelIt != State.Stack.rend()) + for (auto It = ParentLevelIt; It != State.Stack.rend(); ++It) + It->BreakBeforeParameter = true; + } if (PreviousNonComment && !PreviousNonComment->isOneOf(tok::comma, tok::colon, tok::semi) && @@ -1078,7 +1117,10 @@ !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.is(TT_LambdaLBrace))) { CurrentState.BreakBeforeParameter = true; } @@ -1097,7 +1139,7 @@ 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 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -22679,8 +22679,7 @@ " []() -> auto {\n" " int b = 32;\n" " return 3;\n" - " },\n" - " foo, bar)\n" + " }, foo, bar)\n" " .foo();\n" "}", Style); @@ -22706,32 +22705,12 @@ " })));\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" @@ -22739,28 +22718,74 @@ " })));\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; + // 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. + // This should probably be considered a bug. + verifyFormat("void foo() {\n" + " aFunction([](d) -> Foo {\n" + " auto f = e(d);\n" + " return f;\n" + " }, foo, Bar{}, [] {\n" + " auto g = h();\n" + " return g;\n" + " }, baz);\n" + "}", + Style); + // 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"