From 5528cace0402a57564a4b541dcce755a27737d51 Mon Sep 17 00:00:00 2001 From: Krasimir Georgiev Date: Wed, 31 Oct 2018 17:56:57 +0000 Subject: [PATCH] [clang-format] tweaked another case of lambda formatting Summary: This is done in order to improve cases where the lambda's body is moved too far to the right. Consider the following snippet with column limit set to 79: ``` void f() { leader::MakeThisCallHere(&leader_service_, cq_.get(), [this, liveness](const leader::ReadRecordReq& req, std::function done) { logger_->HandleReadRecord( req, resp, std::move(done)); }); leader::MakeAnother(&leader_service_, cq_.get(), [this, liveness](const leader::ReadRecordReq& req, std::function done) { logger_->HandleReadRecord( req, resp, std::move(done), a); }); } ``` The tool favors extra indentation for the lambda body and so the code incurs extra wrapping and adjacent calls are indented to a different level. I find this behavior annoying and I'd like the tool to favor new lines and, thus, use the extra width. The fix, reduced, brings the following formatting. Before: function(1, [] { DoStuff(); // }, 1); After: function( 1, [] { DoStuff(); // }, 1); Refer to the new tests in FormatTest.cpp Contributed by oleg.smolsky! Reviewers: djasper, klimek, krasimir Subscribers: cfe-commits, owenpan Tags: #clang Differential Revision: https://reviews.llvm.org/D52676 llvm-svn: 345753 --- clang/lib/Format/ContinuationIndenter.cpp | 3 +- clang/lib/Format/FormatToken.h | 8 ++ clang/lib/Format/TokenAnnotator.cpp | 24 +++++ clang/lib/Format/UnwrappedLineFormatter.cpp | 7 +- clang/unittests/Format/FormatTest.cpp | 98 ++++++++++++++++++--- 5 files changed, 122 insertions(+), 18 deletions(-) diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index 89d346d8a29c..c369b94b9987 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -1135,7 +1135,8 @@ unsigned ContinuationIndenter::moveStateToNextToken(LineState &State, // }, a, b, c); if (Current.isNot(tok::comment) && Previous && Previous->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) && - !Previous->is(TT_DictLiteral) && State.Stack.size() > 1) { + !Previous->is(TT_DictLiteral) && State.Stack.size() > 1 && + !State.Stack.back().HasMultipleNestedBlocks) { if (State.Stack[State.Stack.size() - 2].NestedBlockInlined && Newline) for (unsigned i = 0, e = State.Stack.size() - 1; i != e; ++i) State.Stack[i].NoLineBreak = true; diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h index 6e801a346b82..10390c42911b 100644 --- a/clang/lib/Format/FormatToken.h +++ b/clang/lib/Format/FormatToken.h @@ -599,6 +599,8 @@ public: /// Notifies the \c Role that a comma was found. virtual void CommaFound(const FormatToken *Token) {} + virtual const FormatToken *lastComma() { return nullptr; } + protected: const FormatStyle &Style; }; @@ -621,6 +623,12 @@ public: Commas.push_back(Token); } + const FormatToken *lastComma() override { + if (Commas.empty()) + return nullptr; + return Commas.back(); + } + private: /// A struct that holds information on how to format a given list with /// a specific number of columns. diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp index d032fd7cb95b..6f9b71b4b1ec 100644 --- a/clang/lib/Format/TokenAnnotator.cpp +++ b/clang/lib/Format/TokenAnnotator.cpp @@ -3049,6 +3049,30 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line, return true; } + // 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 || + Style.Language == FormatStyle::LK_ObjC) && + Left.is(tok::l_paren) && Left.BlockParameterCount > 0 && + !Right.isOneOf(tok::l_paren, TT_LambdaLSquare)) { + // Multiple lambdas in the same function call force line breaks. + if (Left.BlockParameterCount > 1) + return true; + + // A lambda followed by another arg forces a line break. + if (!Left.Role) + return false; + auto Comma = Left.Role->lastComma(); + if (!Comma) + return false; + auto Next = Comma->getNextNonComment(); + if (!Next) + return false; + if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret)) + return true; + } + return false; } diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index 76b495f453fe..6b6a9aff461a 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -988,8 +988,7 @@ private: Path.push_front(Best); Best = Best->Previous; } - for (std::deque::iterator I = Path.begin(), E = Path.end(); - I != E; ++I) { + for (auto I = Path.begin(), E = Path.end(); I != E; ++I) { unsigned Penalty = 0; formatChildren(State, (*I)->NewLine, /*DryRun=*/false, Penalty); Penalty += Indenter->addTokenToState(State, (*I)->NewLine, false); @@ -998,8 +997,8 @@ private: printLineState((*I)->Previous->State); if ((*I)->NewLine) { llvm::dbgs() << "Penalty for placing " - << (*I)->Previous->State.NextToken->Tok.getName() << ": " - << Penalty << "\n"; + << (*I)->Previous->State.NextToken->Tok.getName() + << " on a new line: " << Penalty << "\n"; } }); } diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp index 86b1ca2f17de..635a34499da6 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -3277,11 +3277,12 @@ TEST_F(FormatTest, LayoutNestedBlocks) { "});"); FormatStyle Style = getGoogleStyle(); Style.ColumnLimit = 45; - verifyFormat("Debug(aaaaa,\n" - " {\n" - " if (aaaaaaaaaaaaaaaaaaaaaaaa) return;\n" - " },\n" - " a);", + verifyFormat("Debug(\n" + " aaaaa,\n" + " {\n" + " if (aaaaaaaaaaaaaaaaaaaaaaaa) return;\n" + " },\n" + " a);", Style); verifyFormat("SomeFunction({MACRO({ return output; }), b});"); @@ -11739,6 +11740,18 @@ TEST_F(FormatTest, FormatsLambdas) { " x.end(), //\n" " [&](int, int) { return 1; });\n" "}\n"); + verifyFormat("void f() {\n" + " other.other.other.other.other(\n" + " x.begin(), x.end(),\n" + " [something, rather](int, int, int, int, int, int, int) { return 1; });\n" + "}\n"); + verifyFormat("void f() {\n" + " other.other.other.other.other(\n" + " x.begin(), x.end(),\n" + " [something, rather](int, int, int, int, int, int, int) {\n" + " //\n" + " });\n" + "}\n"); verifyFormat("SomeFunction([]() { // A cool function...\n" " return 43;\n" "});"); @@ -11790,9 +11803,9 @@ TEST_F(FormatTest, FormatsLambdas) { verifyFormat("SomeFunction({[&] {\n" " // comment\n" "}});"); - verifyFormat("virtual aaaaaaaaaaaaaaaa(std::function bbbbbbbbbbbb =\n" - " [&]() { return true; },\n" - " aaaaa aaaaaaaaa);"); + verifyFormat("virtual aaaaaaaaaaaaaaaa(\n" + " std::function bbbbbbbbbbbb = [&]() { return true; },\n" + " aaaaa aaaaaaaaa);"); // Lambdas with return types. verifyFormat("int c = []() -> int { return 2; }();\n"); @@ -11819,17 +11832,76 @@ TEST_F(FormatTest, FormatsLambdas) { " return 1; //\n" "};"); - // Multiple lambdas in the same parentheses change indentation rules. + // Multiple lambdas in the same parentheses change indentation rules. These + // lambdas are forced to start on new lines. verifyFormat("SomeFunction(\n" " []() {\n" - " int i = 42;\n" - " return i;\n" + " //\n" " },\n" " []() {\n" - " int j = 43;\n" - " return j;\n" + " //\n" " });"); + // A lambda passed as arg0 is always pushed to the next line. + verifyFormat("SomeFunction(\n" + " [this] {\n" + " //\n" + " },\n" + " 1);\n"); + + // A multi-line lambda passed as arg1 forces arg0 to be pushed out, just like the arg0 + // case above. + auto Style = getGoogleStyle(); + Style.BinPackArguments = false; + verifyFormat("SomeFunction(\n" + " a,\n" + " [this] {\n" + " //\n" + " },\n" + " b);\n", + Style); + verifyFormat("SomeFunction(\n" + " a,\n" + " [this] {\n" + " //\n" + " },\n" + " b);\n"); + + // A lambda with a very long line forces arg0 to be pushed out irrespective of + // the BinPackArguments value (as long as the code is wide enough). + verifyFormat("something->SomeFunction(\n" + " a,\n" + " [this] {\n" + " D0000000000000000000000000000000000000000000000000000000000001();\n" + " },\n" + " b);\n"); + + // A multi-line lambda is pulled up as long as the introducer fits on the previous + // line and there are no further args. + verifyFormat("function(1, [this, that] {\n" + " //\n" + "});\n"); + verifyFormat("function([this, that] {\n" + " //\n" + "});\n"); + // FIXME: this format is not ideal and we should consider forcing the first arg + // onto its own line. + verifyFormat("function(a, b, c, //\n" + " d, [this, that] {\n" + " //\n" + " });\n"); + + // Multiple lambdas are treated correctly even when there is a short arg0. + verifyFormat("SomeFunction(\n" + " 1,\n" + " [this] {\n" + " //\n" + " },\n" + " [this] {\n" + " //\n" + " },\n" + " 1);\n"); + // More complex introducers. verifyFormat("return [i, args...] {};");