From 5c614bd88f1252927ca1f9d9f8e802ef5e1eebe2 Mon Sep 17 00:00:00 2001 From: Jon Phillips Date: Wed, 5 Apr 2023 14:28:56 -0700 Subject: [PATCH] [clang-format] Fix bugs with "LambdaBodyIndentation: OuterScope" The previous implementation of the option corrupted the parenthesis state stack. (See https://reviews.llvm.org/D102706.) Fixes #55708. Fixes #53212. Fixes #52846. Fixes #59954. Differential Revision: https://reviews.llvm.org/D146042 --- clang/docs/ClangFormatStyleOptions.rst | 11 ++-- clang/docs/ReleaseNotes.rst | 1 + clang/include/clang/Format/Format.h | 11 ++-- clang/lib/Format/ContinuationIndenter.cpp | 14 ++++- clang/lib/Format/UnwrappedLineFormatter.cpp | 11 ---- clang/unittests/Format/FormatTest.cpp | 93 ++++++++++++++++++++++++----- 6 files changed, 104 insertions(+), 37 deletions(-) diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst index 527bdff..f8ab42f 100644 --- a/clang/docs/ClangFormatStyleOptions.rst +++ b/clang/docs/ClangFormatStyleOptions.rst @@ -3540,11 +3540,7 @@ the configuration (without a prefix: ``Auto``). causes the lambda body to be indented one additional level relative to the indentation level of the signature. ``OuterScope`` forces the lambda body to be indented one additional level relative to the parent scope - containing the lambda signature. For callback-heavy code, it may improve - readability to have the signature indented two levels and to use - ``OuterScope``. The KJ style guide requires ``OuterScope``. - `KJ style guide - `_ + containing the lambda signature. Possible values: @@ -3569,6 +3565,11 @@ the configuration (without a prefix: ``Auto``). return; }); + someMethod(someOtherMethod( + [](SomeReallyLongLambdaSignatureArgument foo) { + return; + })); + .. _Language: diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 03d14fc..c07f23a 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -427,6 +427,7 @@ clang-format put the initializers on the next line only. - Add additional Qualifier Ordering support for special cases such as templates, requires clauses, long qualified names. +- Fix all known issues associated with ``LambdaBodyIndentation: OuterScope``. libclang -------- diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h index 755621e..0dfa0528 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -2670,6 +2670,11 @@ struct FormatStyle { /// [](SomeReallyLongLambdaSignatureArgument foo) { /// return; /// }); + /// + /// someMethod(someOtherMethod( + /// [](SomeReallyLongLambdaSignatureArgument foo) { + /// return; + /// })); /// \endcode LBI_OuterScope, }; @@ -2678,11 +2683,7 @@ struct FormatStyle { /// causes the lambda body to be indented one additional level relative to /// the indentation level of the signature. ``OuterScope`` forces the lambda /// body to be indented one additional level relative to the parent scope - /// containing the lambda signature. For callback-heavy code, it may improve - /// readability to have the signature indented two levels and to use - /// ``OuterScope``. The KJ style guide requires ``OuterScope``. - /// `KJ style guide - /// `_ + /// containing the lambda signature. /// \version 13 LambdaBodyIndentationKind LambdaBodyIndentation; diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index 6b33ab0..f5748cf 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -1125,8 +1125,14 @@ unsigned ContinuationIndenter::getNewLineColumn(const LineState &State) { Style.IndentWidth; } - if (NextNonComment->is(tok::l_brace) && NextNonComment->is(BK_Block)) - return Current.NestingLevel == 0 ? State.FirstIndent : CurrentState.Indent; + if (NextNonComment->is(tok::l_brace) && NextNonComment->is(BK_Block)) { + if (Current.NestingLevel == 0 || + (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope && + State.NextToken->is(TT_LambdaLBrace))) { + return State.FirstIndent; + } + return CurrentState.Indent; + } if ((Current.isOneOf(tok::r_brace, tok::r_square) || (Current.is(tok::greater) && (Style.Language == FormatStyle::LK_Proto || @@ -1830,6 +1836,10 @@ void ContinuationIndenter::moveStatePastScopeCloser(LineState &State) { } void ContinuationIndenter::moveStateToNewBlock(LineState &State) { + if (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope && + State.NextToken->is(TT_LambdaLBrace)) { + State.Stack.back().NestedBlockIndent = State.FirstIndent; + } unsigned NestedBlockIndent = State.Stack.back().NestedBlockIndent; // ObjC block sometimes follow special indentation rules. unsigned NewIndent = diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index 97500df..b545fa0 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -1003,17 +1003,6 @@ protected: int AdditionalIndent = P.Indent - Previous.Children[0]->Level * Style.IndentWidth; - - if (Style.LambdaBodyIndentation == FormatStyle::LBI_OuterScope && - P.NestedBlockIndent == P.LastSpace) { - if (State.NextToken->MatchingParen && - State.NextToken->MatchingParen->is(TT_LambdaLBrace)) { - State.Stack.pop_back(); - } - if (LBrace->is(TT_LambdaLBrace)) - AdditionalIndent = 0; - } - Penalty += BlockFormatter->format(Previous.Children, DryRun, AdditionalIndent, /*FixBadIndentation=*/true); diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index 8d009d6..4b7fddd 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -22009,9 +22009,9 @@ TEST_F(FormatTest, FormatsLambdas) { verifyFormat("void test() {\n" " (\n" " []() -> auto {\n" - " int b = 32;\n" - " return 3;\n" - " },\n" + " int b = 32;\n" + " return 3;\n" + " },\n" " foo, bar)\n" " .foo();\n" "}", @@ -22025,17 +22025,82 @@ TEST_F(FormatTest, FormatsLambdas) { " .bar();\n" "}", Style); - Style = getGoogleStyle(); - Style.LambdaBodyIndentation = FormatStyle::LBI_OuterScope; - verifyFormat("#define A \\\n" - " [] { \\\n" - " xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx( \\\n" - " xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx); \\\n" - " }", - Style); - // TODO: The current formatting has a minor issue that's not worth fixing - // right now whereby the closing brace is indented relative to the signature - // instead of being aligned. This only happens with macros. + verifyFormat("#define A \\\n" + " [] { \\\n" + " xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx( \\\n" + " xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx); \\\n" + " }", + Style); + verifyFormat("void foo() {\n" + " aFunction(1, b(c(foo, bar, baz, [](d) {\n" + " auto f = e(d);\n" + " return f;\n" + " })));\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 g = h();\n" + " return g;\n" + " },\n" + " qux,\n" + " [&] -> Bar {\n" + " auto i = j();\n" + " return i;\n" + " });\n" + " return f;\n" + " })));\n" + "}", + Style); + verifyFormat("Namespace::Foo::Foo(\n" + " LongClassName bar, AnotherLongClassName baz)\n" + " : baz{baz}, func{[&] {\n" + " auto qux = bar;\n" + " return aFunkyFunctionCall(qux);\n" + "}} {}", + Style); + 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" + " {\n" + " auto f = e(\n" + " [&]\n" + " {\n" + " auto g = h();\n" + " return g;\n" + " },\n" + " qux,\n" + " [&] -> Bar\n" + " {\n" + " auto i = j();\n" + " return i;\n" + " });\n" + " return f;\n" + " })));\n" + "}", + Style); } TEST_F(FormatTest, LambdaWithLineComments) { -- 2.7.4