jp4a50 created this revision.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay.
jp4a50 requested review of this revision.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D148131
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Format/ContinuationIndenter.cpp
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -22011,8 +22011,7 @@
" []() -> auto {\n"
" int b = 32;\n"
" return 3;\n"
- " },\n"
- " foo, bar)\n"
+ " }, foo, bar)\n"
" .foo();\n"
"}",
Style);
@@ -22038,32 +22037,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"
@@ -22071,28 +22050,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"
Index: clang/lib/Format/TokenAnnotator.cpp
===================================================================
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -5127,7 +5127,8 @@
// Deal with lambda arguments in C++ - we want consistent line breaks whether
// they happen to be at arg0, arg1 or argN. The selection is a bit nuanced
// as aggressive line breaks are placed when the lambda is not the last arg.
- if ((Style.Language == FormatStyle::LK_Cpp ||
+ if (Style.LambdaBodyIndentation == FormatStyle::LBI_Signature &&
+ (Style.Language == FormatStyle::LK_Cpp ||
Style.Language == FormatStyle::LK_ObjC) &&
Left.is(tok::l_paren) && Left.BlockParameterCount > 0 &&
!Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===================================================================
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -309,10 +309,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;
}
@@ -326,6 +327,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;
}
@@ -1022,9 +1027,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) &&
@@ -1034,7 +1071,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;
}
@@ -1053,7 +1093,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 =
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -446,6 +446,8 @@
- Add additional Qualifier Ordering support for special cases such
as templates, requires clauses, long qualified names.
- Fix all known issues associated with ``LambdaBodyIndentation: OuterScope``.
+- Avoid unnecessarily aggressive line-breaking when using
+ ``LambdaBodyIndentation: OuterScope`` with argument bin-packing.
libclang
--------
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
