From: Daniel Jasper Date: Wed, 10 Jul 2013 14:02:49 +0000 (+0000) Subject: Add experimental flag for adaptive parameter bin-packing. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c7bd68f9edcbca95e882d0ab18d09371f0bdb82c;p=clang Add experimental flag for adaptive parameter bin-packing. This is not activated for any style, might change or go away completely. For those that want to play around with it, set ExperimentalAutoDetectBinPacking to true. clang-format will then: Look at whether function calls/declarations/definitions are currently formatted with one parameter per line (on a case-by-case basis). If so, clang-format will avoid bin-packing the parameters. If all parameters are on one line (thus that line is "inconclusive"), clang-format will make the choice dependent on whether there are other bin-packed calls/declarations in the same file. The reason for this change is that bin-packing in some situations can be really bad and an author might opt to put one parameter on each line. If the author does that, he might want clang-format not to mess with that. If the author is unhappy with the one-per-line formatting, clang-format can easily be convinced to bin-pack by putting any two parameters on the same line. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@186003 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Format/Format.h b/include/clang/Format/Format.h index ab3c1aa332..f8786c8555 100644 --- a/include/clang/Format/Format.h +++ b/include/clang/Format/Format.h @@ -77,6 +77,18 @@ struct FormatStyle { /// will either all be on the same line or will have one line each. bool BinPackParameters; + /// \brief If true, clang-format detects whether function calls and + /// definitions are formatted with one parameter per line. + /// + /// Each call can be bin-packed, one-per-line or inconclusive. If it is + /// inconclusive, e.g. completely on one line, but a decision needs to be + /// made, clang-format analyzes whether there are other bin-packed cases in + /// the input file and act accordingly. + /// + /// NOTE: This is an experimental flag, that might go away or be renamed. Do + /// not use this in config files, etc. Use at your own risk. + bool ExperimentalAutoDetectBinPacking; + /// \brief Allow putting all parameters of a function declaration onto /// the next line even if \c BinPackParameters is \c false. bool AllowAllParametersOfDeclarationOnNextLine; @@ -155,6 +167,8 @@ struct FormatStyle { ConstructorInitializerAllOnOneLineOrOnePerLine == R.ConstructorInitializerAllOnOneLineOrOnePerLine && DerivePointerBinding == R.DerivePointerBinding && + ExperimentalAutoDetectBinPacking == + R.ExperimentalAutoDetectBinPacking && IndentCaseLabels == R.IndentCaseLabels && IndentWidth == R.IndentWidth && MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep && diff --git a/lib/Format/Format.cpp b/lib/Format/Format.cpp index b6b726cfb2..ad390c23dc 100644 --- a/lib/Format/Format.cpp +++ b/lib/Format/Format.cpp @@ -94,6 +94,8 @@ template <> struct MappingTraits { IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine", Style.ConstructorInitializerAllOnOneLineOrOnePerLine); IO.mapOptional("DerivePointerBinding", Style.DerivePointerBinding); + IO.mapOptional("ExperimentalAutoDetectBinPacking", + Style.ExperimentalAutoDetectBinPacking); IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels); IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep); IO.mapOptional("ObjCSpaceBeforeProtocolList", @@ -134,6 +136,7 @@ FormatStyle getLLVMStyle() { LLVMStyle.ColumnLimit = 80; LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false; LLVMStyle.DerivePointerBinding = false; + LLVMStyle.ExperimentalAutoDetectBinPacking = false; LLVMStyle.IndentCaseLabels = false; LLVMStyle.MaxEmptyLinesToKeep = 1; LLVMStyle.ObjCSpaceBeforeProtocolList = true; @@ -165,6 +168,7 @@ FormatStyle getGoogleStyle() { GoogleStyle.ColumnLimit = 80; GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true; GoogleStyle.DerivePointerBinding = true; + GoogleStyle.ExperimentalAutoDetectBinPacking = false; GoogleStyle.IndentCaseLabels = true; GoogleStyle.MaxEmptyLinesToKeep = 1; GoogleStyle.ObjCSpaceBeforeProtocolList = false; @@ -260,10 +264,12 @@ public: const AnnotatedLine &Line, unsigned FirstIndent, const FormatToken *RootToken, WhitespaceManager &Whitespaces, - encoding::Encoding Encoding) + encoding::Encoding Encoding, + bool BinPackInconclusiveFunctions) : Style(Style), SourceMgr(SourceMgr), Line(Line), FirstIndent(FirstIndent), RootToken(RootToken), - Whitespaces(Whitespaces), Count(0), Encoding(Encoding) {} + Whitespaces(Whitespaces), Count(0), Encoding(Encoding), + BinPackInconclusiveFunctions(BinPackInconclusiveFunctions) {} /// \brief Formats an \c UnwrappedLine. void format(const AnnotatedLine *NextLine) { @@ -782,7 +788,11 @@ private: } else { NewIndent = 4 + std::max(LastSpace, State.Stack.back().StartOfFunctionCall); - AvoidBinPacking = !Style.BinPackParameters; + AvoidBinPacking = !Style.BinPackParameters || + (Style.ExperimentalAutoDetectBinPacking && + (Current.PackingKind == PPK_OnePerLine || + (!BinPackInconclusiveFunctions && + Current.PackingKind == PPK_Inconclusive))); } State.Stack.push_back(ParenState(NewIndent, LastSpace, AvoidBinPacking, @@ -1157,6 +1167,7 @@ private: // to create a deterministic order independent of the container. unsigned Count; encoding::Encoding Encoding; + bool BinPackInconclusiveFunctions; }; class FormatTokenLexer { @@ -1363,7 +1374,8 @@ public: 1; } UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent, - TheLine.First, Whitespaces, Encoding); + TheLine.First, Whitespaces, Encoding, + BinPackInconclusiveFunctions); Formatter.format(I + 1 != E ? &*(I + 1) : NULL); IndentForLevel[TheLine.Level] = LevelIndent; PreviousLineWasTouched = true; @@ -1408,6 +1420,8 @@ private: unsigned CountBoundToVariable = 0; unsigned CountBoundToType = 0; bool HasCpp03IncompatibleFormat = false; + bool HasBinPackedFunction = false; + bool HasOnePerLineFunction = false; for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) { if (!AnnotatedLines[i].First->Next) continue; @@ -1428,6 +1442,12 @@ private: Tok->Previous->Type == TT_TemplateCloser && Tok->WhitespaceRange.getBegin() == Tok->WhitespaceRange.getEnd()) HasCpp03IncompatibleFormat = true; + + if (Tok->PackingKind == PPK_BinPacked) + HasBinPackedFunction = true; + if (Tok->PackingKind == PPK_OnePerLine) + HasOnePerLineFunction = true; + Tok = Tok->Next; } } @@ -1441,6 +1461,8 @@ private: Style.Standard = HasCpp03IncompatibleFormat ? FormatStyle::LS_Cpp11 : FormatStyle::LS_Cpp03; } + BinPackInconclusiveFunctions = + HasBinPackedFunction || !HasOnePerLineFunction; } /// \brief Get the indent of \p Level from \p IndentForLevel. @@ -1691,6 +1713,7 @@ private: std::vector AnnotatedLines; encoding::Encoding Encoding; + bool BinPackInconclusiveFunctions; }; } // end anonymous namespace diff --git a/lib/Format/FormatToken.h b/lib/Format/FormatToken.h index 1de4b8964f..03f097000f 100644 --- a/lib/Format/FormatToken.h +++ b/lib/Format/FormatToken.h @@ -64,6 +64,13 @@ enum BraceBlockKind { BK_BracedInit }; +// The packing kind of a function's parameters. +enum ParameterPackingKind { + PPK_BinPacked, + PPK_OnePerLine, + PPK_Inconclusive +}; + /// \brief A wrapper around a \c Token storing information about the /// whitespace characters preceeding it. struct FormatToken { @@ -72,9 +79,9 @@ struct FormatToken { CodePointCount(0), IsFirst(false), MustBreakBefore(false), BlockKind(BK_Unknown), Type(TT_Unknown), SpacesRequiredBefore(0), CanBreakBefore(false), ClosesTemplateDeclaration(false), - ParameterCount(0), TotalLength(0), UnbreakableTailLength(0), - BindingStrength(0), SplitPenalty(0), LongestObjCSelectorName(0), - FakeRParens(0), LastInChainOfCalls(false), + ParameterCount(0), PackingKind(PPK_Inconclusive), TotalLength(0), + UnbreakableTailLength(0), BindingStrength(0), SplitPenalty(0), + LongestObjCSelectorName(0), FakeRParens(0), LastInChainOfCalls(false), PartOfMultiVariableDeclStmt(false), MatchingParen(NULL), Previous(NULL), Next(NULL) {} @@ -143,6 +150,9 @@ struct FormatToken { /// the number of commas. unsigned ParameterCount; + /// \brief If this is an opening parenthesis, how are the parameters packed? + ParameterPackingKind PackingKind; + /// \brief The total length of the line up to and including this token. unsigned TotalLength; diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp index ce2db6d9c0..f5b6fbde7c 100644 --- a/lib/Format/TokenAnnotator.cpp +++ b/lib/Format/TokenAnnotator.cpp @@ -99,6 +99,8 @@ private: } bool MightBeFunctionType = CurrentToken->is(tok::star); + bool HasMultipleLines = false; + bool HasMultipleParametersOnALine = false; while (CurrentToken != NULL) { // LookForDecls is set when "if (" has been seen. Check for // 'identifier' '*' 'identifier' followed by not '=' -- this @@ -133,6 +135,13 @@ private: } } + if (!HasMultipleLines) + Left->PackingKind = PPK_Inconclusive; + else if (HasMultipleParametersOnALine) + Left->PackingKind = PPK_BinPacked; + else + Left->PackingKind = PPK_OnePerLine; + next(); return true; } @@ -143,8 +152,14 @@ private: tok::coloncolon)) MightBeFunctionType = true; updateParameterCount(Left, CurrentToken); + if (CurrentToken->is(tok::comma) && CurrentToken->Next && + !CurrentToken->Next->HasUnescapedNewline && + !CurrentToken->Next->isTrailingComment()) + HasMultipleParametersOnALine = true; if (!consumeToken()) return false; + if (CurrentToken && CurrentToken->HasUnescapedNewline) + HasMultipleLines = true; } return false; } @@ -245,10 +260,11 @@ private: } void updateParameterCount(FormatToken *Left, FormatToken *Current) { - if (Current->is(tok::comma)) + if (Current->is(tok::comma)) { ++Left->ParameterCount; - else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) + } else if (Left->ParameterCount == 0 && Current->isNot(tok::comment)) { Left->ParameterCount = 1; + } } bool parseConditional() { @@ -1283,9 +1299,10 @@ void TokenAnnotator::printDebugInfo(const AnnotatedLine &Line) { const FormatToken *Tok = Line.First; while (Tok) { llvm::errs() << " M=" << Tok->MustBreakBefore - << " C=" << Tok->CanBreakBefore << " T=" << Tok->Type << " S=" - << Tok->SpacesRequiredBefore << " P=" << Tok->SplitPenalty - << " Name=" << Tok->Tok.getName() << " FakeLParens="; + << " C=" << Tok->CanBreakBefore << " T=" << Tok->Type + << " S=" << Tok->SpacesRequiredBefore + << " P=" << Tok->SplitPenalty << " Name=" << Tok->Tok.getName() + << " PPK=" << Tok->PackingKind << " FakeLParens="; for (unsigned i = 0, e = Tok->FakeLParens.size(); i != e; ++i) llvm::errs() << Tok->FakeLParens[i] << "/"; llvm::errs() << " FakeRParens=" << Tok->FakeRParens << "\n"; diff --git a/unittests/Format/FormatTest.cpp b/unittests/Format/FormatTest.cpp index 0192a0718b..b87393a9da 100644 --- a/unittests/Format/FormatTest.cpp +++ b/unittests/Format/FormatTest.cpp @@ -2644,6 +2644,30 @@ TEST_F(FormatTest, FormatsOneParameterPerLineIfNecessary) { NoBinPacking); } +TEST_F(FormatTest, AdaptiveOnePerLineFormatting) { + FormatStyle Style = getLLVMStyleWithColumns(15); + Style.ExperimentalAutoDetectBinPacking = true; + EXPECT_EQ("aaa(aaaa,\n" + " aaaa,\n" + " aaaa);\n" + "aaa(aaaa,\n" + " aaaa,\n" + " aaaa);", + format("aaa(aaaa,\n" // one-per-line + " aaaa,\n" + " aaaa );\n" + "aaa(aaaa, aaaa, aaaa);", // inconclusive + Style)); + EXPECT_EQ("aaa(aaaa, aaaa,\n" + " aaaa);\n" + "aaa(aaaa, aaaa,\n" + " aaaa);", + format("aaa(aaaa, aaaa,\n" // bin-packed + " aaaa );\n" + "aaa(aaaa, aaaa, aaaa);", // inconclusive + Style)); +} + TEST_F(FormatTest, FormatsBuilderPattern) { verifyFormat( "return llvm::StringSwitch(name)\n"