From 37c2233097ac44697b87228d86eef1fce10ea5c1 Mon Sep 17 00:00:00 2001 From: mydeveloperday Date: Sat, 26 Jun 2021 13:34:07 +0100 Subject: [PATCH] [clang-format] [PR50702] Lamdba processing does not respect AfterClass and AfterNamespace https://bugs.llvm.org/show_bug.cgi?id=50702 I believe {D44609} may be too aggressive with brace wrapping rules which doesn't always apply to Lamdbas The introduction of BeforeLambdaBody and AllowShortLambdasOnASingleLine has impact on brace handling on other block types, which I suspect we didn't see before as people may not be using the BeforeLambdaBody style From what I can tell this can be seen by the unit test I change as its not honouring the orginal LLVM brace wrapping style for the `Fct()` function I added a unit test from PR50702 and have removed some of the code (which has zero impact on the unit test, which kind of suggests its unnecessary), some additional attempt has been made to try and ensure we'll only break on what is actually a LamdbaLBrace Reviewed By: HazardyKnusperkeks Differential Revision: https://reviews.llvm.org/D104222 --- clang/lib/Format/TokenAnnotator.cpp | 46 +++++------------------------------ clang/unittests/Format/FormatTest.cpp | 32 ++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp index bfa49de..5f97395 100644 --- a/clang/lib/Format/TokenAnnotator.cpp +++ b/clang/lib/Format/TokenAnnotator.cpp @@ -3579,42 +3579,11 @@ isItAnEmptyLambdaAllowed(const FormatToken &Tok, return Tok.Children.empty() && ShortLambdaOption != FormatStyle::SLS_None; } -static bool -isItAInlineLambdaAllowed(const FormatToken &Tok, - FormatStyle::ShortLambdaStyle ShortLambdaOption) { - return (ShortLambdaOption == FormatStyle::SLS_Inline && - IsFunctionArgument(Tok)) || - (ShortLambdaOption == FormatStyle::SLS_All); -} - -static bool isOneChildWithoutMustBreakBefore(const FormatToken &Tok) { - if (Tok.Children.size() != 1) - return false; - FormatToken *curElt = Tok.Children[0]->First; - while (curElt) { - if (curElt->MustBreakBefore) - return false; - curElt = curElt->Next; - } - return true; -} static bool isAllmanLambdaBrace(const FormatToken &Tok) { return (Tok.is(tok::l_brace) && Tok.is(BK_Block) && !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral)); } -static bool isAllmanBraceIncludedBreakableLambda( - const FormatToken &Tok, FormatStyle::ShortLambdaStyle ShortLambdaOption) { - if (!isAllmanLambdaBrace(Tok)) - return false; - - if (isItAnEmptyLambdaAllowed(Tok, ShortLambdaOption)) - return false; - - return !isItAInlineLambdaAllowed(Tok, ShortLambdaOption) || - !isOneChildWithoutMustBreakBefore(Tok); -} - bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line, const FormatToken &Right) { const FormatToken &Left = *Right.Previous; @@ -3776,13 +3745,6 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line, if (Right.is(TT_InlineASMBrace)) return Right.HasUnescapedNewline; - auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine; - if (Style.BraceWrapping.BeforeLambdaBody && - (isAllmanBraceIncludedBreakableLambda(Left, ShortLambdaOption) || - isAllmanBraceIncludedBreakableLambda(Right, ShortLambdaOption))) { - return true; - } - if (isAllmanBrace(Left) || isAllmanBrace(Right)) return (Line.startsWith(tok::kw_enum) && Style.BraceWrapping.AfterEnum) || (Line.startsWith(tok::kw_typedef, tok::kw_enum) && @@ -3805,6 +3767,11 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line, return true; } + if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace) && + Left.isOneOf(tok::star, tok::amp, tok::ampamp, TT_TemplateCloser)) { + return true; + } + // Put multiple Java annotation on a new line. if ((Style.Language == FormatStyle::LK_Java || Style.Language == FormatStyle::LK_JavaScript) && @@ -4209,7 +4176,7 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line, return false; auto ShortLambdaOption = Style.AllowShortLambdasOnASingleLine; - if (Style.BraceWrapping.BeforeLambdaBody) { + if (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) { if (isAllmanLambdaBrace(Left)) return !isItAnEmptyLambdaAllowed(Left, ShortLambdaOption); if (isAllmanLambdaBrace(Right)) @@ -4221,7 +4188,6 @@ bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line, Right.isMemberAccess() || Right.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow, tok::lessless, tok::colon, tok::l_square, tok::at) || - (Style.BraceWrapping.BeforeLambdaBody && Right.is(TT_LambdaLBrace)) || (Left.is(tok::r_paren) && Right.isOneOf(tok::identifier, tok::kw_const)) || (Left.is(tok::l_paren) && !Right.is(tok::r_paren)) || diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index e084d06..e5fa172 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -19596,8 +19596,7 @@ TEST_F(FormatTest, FormatsLambdas) { " });\n" " });", LLVMWithBeforeLambdaBody); - verifyFormat("void Fct()\n" - "{\n" + verifyFormat("void Fct() {\n" " return {[]()\n" " {\n" " return 17;\n" @@ -19802,6 +19801,35 @@ TEST_F(FormatTest, FormatsLambdas) { " });", LLVMWithBeforeLambdaBody); + LLVMWithBeforeLambdaBody.AllowShortLambdasOnASingleLine = + FormatStyle::ShortLambdaStyle::SLS_None; + + verifyFormat("auto select = [this]() -> const Library::Object *\n" + "{\n" + " return MyAssignment::SelectFromList(this);\n" + "};\n", + LLVMWithBeforeLambdaBody); + + verifyFormat("auto select = [this]() -> const Library::Object &\n" + "{\n" + " return MyAssignment::SelectFromList(this);\n" + "};\n", + LLVMWithBeforeLambdaBody); + + verifyFormat("auto select = [this]() -> std::unique_ptr\n" + "{\n" + " return MyAssignment::SelectFromList(this);\n" + "};\n", + LLVMWithBeforeLambdaBody); + + verifyFormat("namespace test {\n" + "class Test {\n" + "public:\n" + " Test() = default;\n" + "};\n" + "} // namespace test", + LLVMWithBeforeLambdaBody); + // Lambdas with different indentation styles. Style = getLLVMStyleWithColumns(100); EXPECT_EQ("SomeResult doSomething(SomeObject promise) {\n" -- 2.7.4