mxbOctasic created this revision. mxbOctasic added a reviewer: djasper. mxbOctasic added subscribers: cameron314, cfe-commits. Herald added a subscriber: klimek.
Multiple brace wrapping flag combination were broken. First, IndentBraces + !AfterControlStatement caused the whole If-Else construct to be indented. Currently, only function blocks can be merged into one line when the opening brace wrapped. The short block merging logic checks the AfterFunction flag for all block types. With AllowShortFunctions (All), !BraceWrapping.AfterFunction and BraceWrapping.AfterControlStatement I ended up with this: if(true) {} This happened with all other non-function blocks with the opening brace wrapped on its own line. http://reviews.llvm.org/D19069 Files: lib/Format/FormatToken.h lib/Format/TokenAnnotator.cpp lib/Format/UnwrappedLineFormatter.cpp lib/Format/UnwrappedLineParser.cpp unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp =================================================================== --- unittests/Format/FormatTest.cpp +++ unittests/Format/FormatTest.cpp @@ -423,6 +423,119 @@ " f();\n" "}", AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.BreakBeforeBraces = FormatStyle::BS_Custom; + AllowSimpleBracedStatements.BraceWrapping.AfterControlStatement = true; + AllowSimpleBracedStatements.BraceWrapping.BeforeElse = true; + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = false; + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false; + + verifyFormat("if (true)\n" + "{\n" + " f();\n" + "}\n" + "else\n" + "{\n" + " f();\n" + "}\n", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = false; + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true; + + verifyFormat("if (true)\n" + "{\n" + " f();\n" + "}\n" + "else\n" + "{\n" + " f();\n" + "}\n", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true; + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = false; + + verifyFormat("if (true)\n" + "{\n" + " f();\n" + "}\n" + "else\n" + "{\n" + " f();\n" + "}\n", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true; + AllowSimpleBracedStatements.AllowShortIfStatementsOnASingleLine = true; + + EXPECT_EQ("if (true) { f(); }\nelse { f(); }", + format("if (true)\n" + "{\n" + " f();\n" + "}\n" + "else\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements)); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = false; + AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = false; + + verifyFormat("while (true)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + verifyFormat("for (;;)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = false; + AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true; + + verifyFormat("while (true)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + verifyFormat("for (;;)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true; + AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = false; + + verifyFormat("while (true)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + verifyFormat("for (;;)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements); + + AllowSimpleBracedStatements.AllowShortBlocksOnASingleLine = true; + AllowSimpleBracedStatements.AllowShortLoopsOnASingleLine = true; + + EXPECT_EQ("while (true) { f(); }", format("while (true)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements)); + EXPECT_EQ("for (;;) { f(); }", format("for (;;)\n" + "{\n" + " f();\n" + "}", + AllowSimpleBracedStatements)); } TEST_F(FormatTest, ParseIfElse) { @@ -2338,7 +2451,9 @@ "}"); // Function-level try statements. - verifyFormat("int f() try { return 4; } catch (...) {\n" + verifyFormat("int f() try {\n" + " return 4;\n" + "} catch (...) {\n" " return 5;\n" "}"); verifyFormat("class A {\n" @@ -2995,7 +3110,7 @@ } TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) { - verifyFormat("{\n { a #c; }\n}"); + verifyFormat("{\n {\n a #c;\n }\n}"); } TEST_F(FormatTest, FormatUnbalancedStructuralElements) { @@ -6007,7 +6122,7 @@ // FIXME: single value wrapped with paren will be treated as cast. verifyFormat("void f(int i = (kValue)*kMask) {}"); - verifyFormat("{ (void)F; }"); + verifyFormat("{\n (void)F;\n}"); // Don't break after a cast's verifyFormat("int aaaaaaaaaaaaaaaaaaaaaaaaaaa =\n" @@ -6797,6 +6912,22 @@ " void f() { int i; } \\\n" " int j;", getLLVMStyleWithColumns(23)); + + FormatStyle CustomBraceStyle = getLLVMStyle(); + CustomBraceStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; + CustomBraceStyle.BreakBeforeBraces = FormatStyle::BS_Custom; + CustomBraceStyle.BraceWrapping.AfterFunction = true; + + verifyFormat("void empty() {}", CustomBraceStyle); + + EXPECT_EQ("void empty() {}", format("void empty()\n" + "{\n" + "}", + CustomBraceStyle)); + + EXPECT_EQ("void empty() {}", format("void empty() {\n" + "}", + CustomBraceStyle)); } TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) { @@ -9743,6 +9874,128 @@ WebKitBraceStyle); } +TEST_F(FormatTest, IndentBracesBreaking) { + FormatStyle Style = getLLVMStyle(); + + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.IndentBraces = true; + + Style.BraceWrapping.AfterControlStatement = true; + Style.BraceWrapping.BeforeElse = false; + Style.BraceWrapping.AfterFunction = true; + verifyFormat("void A(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + "}", + Style); + verifyFormat("void B(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + " else\n" + " {\n" + " Funk(rand() == 2);\n" + " }\n" + "}", + Style); + verifyFormat("void C(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + " else\n" + " Funk(rand() == 2);\n" + "}", + Style); + + Style.BraceWrapping.BeforeElse = true; + verifyFormat("void D(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + "}", + Style); + verifyFormat("void E(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + " else\n" + " {\n" + " Funk(rand() == 2);\n" + " }\n" + "}", + Style); + verifyFormat("void F(bool b)\n" + "{\n" + " if (b)\n" + " {\n" + " Funk(rand() == 4);\n" + " }\n" + " else\n" + " Funk(rand() == 2);\n" + "}", + Style); + + Style.BraceWrapping.AfterControlStatement = false; + Style.BraceWrapping.BeforeElse = false; + Style.BraceWrapping.AfterFunction = false; + verifyFormat("void G(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " }\n" + "}", + Style); + verifyFormat("void H(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " } else {\n" + " Funk(rand() == 2);\n" + " }\n" + "}", + Style); + verifyFormat("void I(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " } else\n" + " Funk(rand() == 2);\n" + "}", + Style); + Style.BraceWrapping.BeforeElse = true; + verifyFormat("void J(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " }\n" + "}", + Style); + verifyFormat("void K(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " }\n" + " else {\n" + " Funk(rand() == 2);\n" + " }\n" + "}", + Style); + verifyFormat("void L(bool b) {\n" + " if (b) {\n" + " Funk(rand() == 4);\n" + " }\n" + " else\n" + " Funk(rand() == 2);\n" + "}", + Style); +} + TEST_F(FormatTest, CatchExceptionReferenceBinding) { verifyFormat("void f() {\n" " try {\n" Index: lib/Format/UnwrappedLineParser.cpp =================================================================== --- lib/Format/UnwrappedLineParser.cpp +++ lib/Format/UnwrappedLineParser.cpp @@ -154,14 +154,29 @@ CompoundStatementIndenter(UnwrappedLineParser *Parser, const FormatStyle &Style, unsigned &LineLevel) : LineLevel(LineLevel), OldLineLevel(LineLevel) { - if (Style.BraceWrapping.AfterControlStatement) - Parser->addUnwrappedLine(); - if (Style.BraceWrapping.IndentBraces) - ++LineLevel; + init(Parser, Style, Style.BraceWrapping.AfterControlStatement); + } + CompoundStatementIndenter(UnwrappedLineParser *Parser, + const FormatStyle &Style, unsigned &LineLevel, + bool wrapBrace) + : LineLevel(LineLevel), OldLineLevel(LineLevel) { + init(Parser, Style, wrapBrace); } ~CompoundStatementIndenter() { LineLevel = OldLineLevel; } private: + void init(UnwrappedLineParser *Parser, const FormatStyle &Style, + bool wrapBrace) { + assert(Parser->FormatTok->is(tok::l_brace)); + Parser->FormatTok->Type = TT_CompoundStatementLBrace; + if (wrapBrace) { + Parser->addUnwrappedLine(); + if (Style.BraceWrapping.IndentBraces) + ++LineLevel; + } + } + +private: unsigned &LineLevel; unsigned OldLineLevel; }; @@ -771,8 +786,8 @@ case tok::objc_autoreleasepool: nextToken(); if (FormatTok->Tok.is(tok::l_brace)) { - if (Style.BraceWrapping.AfterObjCDeclaration) - addUnwrappedLine(); + CompoundStatementIndenter Indenter( + this, Style, Line->Level, Style.BraceWrapping.AfterObjCDeclaration); parseBlock(/*MustBeDeclaration=*/false); } addUnwrappedLine(); @@ -1370,7 +1385,9 @@ if (FormatTok->Tok.is(tok::l_brace)) { CompoundStatementIndenter Indenter(this, Style, Line->Level); parseBlock(/*MustBeDeclaration=*/false); - if (Style.BraceWrapping.BeforeElse) + if (Style.BraceWrapping.BeforeElse || + Style.BraceWrapping.AfterControlStatement && + Style.BraceWrapping.IndentBraces) addUnwrappedLine(); else NeedsUnwrappedLine = true; @@ -1520,11 +1537,16 @@ void UnwrappedLineParser::parseForOrWhileLoop() { assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) && "'for', 'while' or foreach macro expected"); + bool isMacro = FormatTok->is(TT_ForEachMacro); nextToken(); if (FormatTok->Tok.is(tok::l_paren)) parseParens(); if (FormatTok->Tok.is(tok::l_brace)) { CompoundStatementIndenter Indenter(this, Style, Line->Level); + // FIXME: TT_ForEachMacro not treated like tok::kw_for by the rest of + // system. + if (isMacro) + FormatTok->Type = TT_Unknown; parseBlock(/*MustBeDeclaration=*/false); addUnwrappedLine(); } else { @@ -1791,6 +1813,9 @@ } } if (FormatTok->Tok.is(tok::l_brace)) { + if (InitialToken.isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct, + Keywords.kw_interface)) + FormatTok->Type = TT_RecordLBrace; if (ShouldBreakBeforeBrace(Style, InitialToken)) addUnwrappedLine(); Index: lib/Format/UnwrappedLineFormatter.cpp =================================================================== --- lib/Format/UnwrappedLineFormatter.cpp +++ lib/Format/UnwrappedLineFormatter.cpp @@ -185,50 +185,34 @@ ? 0 : Limit - TheLine->Last->TotalLength; - // FIXME: TheLine->Level != 0 might or might not be the right check to do. - // If necessary, change to something smarter. - bool MergeShortFunctions = - Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All || - (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty && - I[1]->First->is(tok::r_brace)) || - (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline && - TheLine->Level != 0); - if (TheLine->Last->is(TT_FunctionLBrace) && - TheLine->First != TheLine->Last) { + TheLine->First != TheLine->Last || + I[1]->First->is(TT_FunctionLBrace) && I[1]->First == I[1]->Last) { + // FIXME: TheLine->Level != 0 might or might not be the right check to do. + // If necessary, change to something smarter. + bool MergeShortFunctions = + Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All || + (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty && + (TheLine->Last->is(tok::l_brace) && I[1]->First->is(tok::r_brace) || + !TheLine->Last->is(tok::l_brace) && I[1]->First->is(tok::l_brace) && + I + 2 != E && I[2]->First->is(tok::r_brace))) || + (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline && + TheLine->Level != 0); return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0; } - if (TheLine->Last->is(tok::l_brace)) { - return !Style.BraceWrapping.AfterFunction - ? tryMergeSimpleBlock(I, E, Limit) - : 0; + if (TheLine->Last->is(TT_RecordLBrace) && TheLine->First != TheLine->Last || + I[1]->First->is(TT_RecordLBrace)) { + return tryMergeSimpleBlock(I, E, Limit); } - if (I[1]->First->is(TT_FunctionLBrace) && - Style.BraceWrapping.AfterFunction) { - if (I[1]->Last->is(TT_LineComment)) - return 0; - - // Check for Limit <= 2 to account for the " {". - if (Limit <= 2 || (Style.ColumnLimit == 0 && containsMustBreak(TheLine))) - return 0; - Limit -= 2; - - unsigned MergedLines = 0; - if (MergeShortFunctions) { - MergedLines = tryMergeSimpleBlock(I + 1, E, Limit); - // If we managed to merge the block, count the function header, which is - // on a separate line. - if (MergedLines > 0) - ++MergedLines; - } - return MergedLines; + if (TheLine->Last->is(tok::l_brace) && TheLine->Last->is(TT_Unknown)) { + return tryMergeSimpleBlock(I, E, Limit); } - if (TheLine->First->is(tok::kw_if)) { + if (TheLine->First->isOneOf(tok::kw_if, tok::kw_else)) { return Style.AllowShortIfStatementsOnASingleLine ? tryMergeSimpleControlStatement(I, E, Limit) : 0; } - if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while)) { + if (TheLine->First->isOneOf(tok::kw_for, tok::kw_while, tok::kw_do)) { return Style.AllowShortLoopsOnASingleLine ? tryMergeSimpleControlStatement(I, E, Limit) : 0; @@ -263,14 +247,15 @@ SmallVectorImpl<AnnotatedLine *>::const_iterator E, unsigned Limit) { if (Limit == 0) return 0; - if (Style.BraceWrapping.AfterControlStatement && - (I[1]->First->is(tok::l_brace) && !Style.AllowShortBlocksOnASingleLine)) - return 0; if (I[1]->InPPDirective != (*I)->InPPDirective || (I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) return 0; Limit = limitConsideringMacros(I + 1, E, Limit); AnnotatedLine &Line = **I; + if (I[0]->Last->is(TT_CompoundStatementLBrace) || + I[1]->First->is(TT_CompoundStatementLBrace) && + I[1]->First == I[1]->Last) + return tryMergeSimpleBlock(I, E, Limit); if (Line.Last->isNot(tok::r_paren)) return 0; if (1 + I[1]->Last->TotalLength > Limit) @@ -292,6 +277,9 @@ if (Limit == 0 || I + 1 == E || I[1]->First->isOneOf(tok::kw_case, tok::kw_default)) return 0; + if (I[0]->Last->is(tok::l_brace) || + I[1]->First->is(tok::l_brace) && I[1]->First == I[1]->Last) + return 0; unsigned NumStmts = 0; unsigned Length = 0; bool InPPDirective = I[0]->InPPDirective; @@ -320,54 +308,61 @@ unsigned Limit) { AnnotatedLine &Line = **I; + // FIXME: Consider an option to allow short exception handling clauses on + // a single line. + // FIXME: This isn't covered by tests. + // FIXME: For catch, __except, __finally the first token on the line + // is '}', so this isn't correct here. + if (Line.First->isOneOf(tok::kw_try, tok::kw___try, tok::kw_catch, + Keywords.kw___except, tok::kw___finally)) + return 0; + // Don't merge ObjC @ keywords and methods. // FIXME: If an option to allow short exception handling clauses on a single // line is added, change this to not return for @try and friends. if (Style.Language != FormatStyle::LK_Java && Line.First->isOneOf(tok::at, tok::minus, tok::plus)) return 0; - // Check that the current line allows merging. This depends on whether we - // are in a control flow statements as well as several style flags. - if (Line.First->isOneOf(tok::kw_else, tok::kw_case) || - (Line.First->Next && Line.First->Next->is(tok::kw_else))) - return 0; - if (Line.First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_do, tok::kw_try, - tok::kw___try, tok::kw_catch, tok::kw___finally, - tok::kw_for, tok::r_brace, Keywords.kw___except)) { - if (!Style.AllowShortBlocksOnASingleLine) - return 0; - if (!Style.AllowShortIfStatementsOnASingleLine && - Line.startsWith(tok::kw_if)) - return 0; - if (!Style.AllowShortLoopsOnASingleLine && - Line.First->isOneOf(tok::kw_while, tok::kw_do, tok::kw_for)) - return 0; - // FIXME: Consider an option to allow short exception handling clauses on - // a single line. - // FIXME: This isn't covered by tests. - // FIXME: For catch, __except, __finally the first token on the line - // is '}', so this isn't correct here. - if (Line.First->isOneOf(tok::kw_try, tok::kw___try, tok::kw_catch, - Keywords.kw___except, tok::kw___finally)) + // There is no brace on this line, check if the opening brace is on the + // next and belong to a construct (compound statement, function or record). + // Move the iterator to merge the block with the first line. + // Ex: if (1) { exit 0; } + if (Line.Last->isNot(tok::l_brace) && I[1]->First == I[1]->Last && + I[1]->Last->is(tok::l_brace) && I[1]->Last->isNot(TT_Unknown)) { + if (I + 1 == E || Limit <= 2 || + (Style.ColumnLimit == 0 && containsMustBreak(&Line))) return 0; + Limit -= 2; + ++I; } - + assert(I[0]->Last->is(tok::l_brace)); + FormatToken *BlockLBrace = I[0]->Last; FormatToken *Tok = I[1]->First; + + if (BlockLBrace->is(TT_CompoundStatementLBrace) && + !Style.AllowShortBlocksOnASingleLine) + return 0; + if (Tok->is(tok::r_brace) && !Tok->MustBreakBefore && (Tok->getNextNonComment() == nullptr || Tok->getNextNonComment()->is(tok::semi))) { // We merge empty blocks even if the line exceeds the column limit. Tok->SpacesRequiredBefore = 0; Tok->CanBreakBefore = true; - return 1; + return BlockLBrace == Line.Last ? 1 : 2; } else if (Limit != 0 && !Line.startsWith(tok::kw_namespace) && !startsExternCBlock(Line)) { // We don't merge short records. if (Line.First->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct, Keywords.kw_interface)) return 0; + if (BlockLBrace->isNot(TT_FunctionLBrace) && + Line.First->isNot(tok::kw_export) && + !Style.AllowShortBlocksOnASingleLine) + return 0; + // Check that we still have three lines and they fit into the limit. if (I + 2 == E || I[2]->Type == LT_Invalid) return 0; @@ -395,7 +390,7 @@ if (Tok->Next && Tok->Next->is(tok::kw_else)) return 0; - return 2; + return BlockLBrace == Line.Last ? 2 : 3; } return 0; } Index: lib/Format/TokenAnnotator.cpp =================================================================== --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -828,7 +828,8 @@ if (!CurrentToken->isOneOf(TT_LambdaLSquare, TT_ForEachMacro, TT_FunctionLBrace, TT_ImplicitStringLiteral, TT_InlineASMBrace, TT_JsFatArrow, TT_LambdaArrow, - TT_RegexLiteral)) + TT_RegexLiteral, TT_CompoundStatementLBrace, + TT_RecordLBrace)) CurrentToken->Type = TT_Unknown; CurrentToken->Role.reset(); CurrentToken->MatchingParen = nullptr; Index: lib/Format/FormatToken.h =================================================================== --- lib/Format/FormatToken.h +++ lib/Format/FormatToken.h @@ -37,6 +37,7 @@ TYPE(ConflictAlternative) \ TYPE(ConflictEnd) \ TYPE(ConflictStart) \ + TYPE(CompoundStatementLBrace) \ TYPE(CtorInitializerColon) \ TYPE(CtorInitializerComma) \ TYPE(DesignatedInitializerPeriod) \ @@ -75,6 +76,7 @@ TYPE(PointerOrReference) \ TYPE(PureVirtualSpecifier) \ TYPE(RangeBasedForLoopColon) \ + TYPE(RecordLBrace) \ TYPE(RegexLiteral) \ TYPE(SelectorName) \ TYPE(StartOfName) \
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits