[cfe-commits] r170457 - in /cfe/trunk: lib/Format/Format.cpp unittests/Format/FormatTest.cpp
Daniel Jasper
djasper at google.com
Tue Dec 18 13:16:47 PST 2012
Also, I entirely removed the penalty for the extra line count as it turns
out that the actual line count is not really correlated with readability.
Also, due to the split penalty in the different cases, clang-format already
keeps the line count low.
This was necessary to fix the test case with the corresponding comment.
On Tue, Dec 18, 2012 at 10:05 PM, Daniel Jasper <djasper at google.com> wrote:
> Author: djasper
> Date: Tue Dec 18 15:05:13 2012
> New Revision: 170457
>
> URL: http://llvm.org/viewvc/llvm-project?rev=170457&view=rev
> Log:
> Better support for constructor initializers.
>
> We used to format initializers like this (with a sort of hacky
> implementation):
> Constructor()
> : Val1(A),
> Val2(B) {
>
> and now format like this (with a somewhat better solution):
> Constructor()
> : Val1(A), Val2(B) {
>
> assuming this would not fit on a single line. Also added tests.
>
> As a side effect we now first analyze whether an UnwrappedLine needs to be
> split at all. If not, not splitting it is the best solution by definition.
> As
> this should be a very common case in normal code, not exploring the entire
> solution space can provide significant speedup.
>
> Modified:
> cfe/trunk/lib/Format/Format.cpp
> cfe/trunk/unittests/Format/FormatTest.cpp
>
> Modified: cfe/trunk/lib/Format/Format.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=170457&r1=170456&r2=170457&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Format/Format.cpp (original)
> +++ cfe/trunk/lib/Format/Format.cpp Tue Dec 18 15:05:13 2012
> @@ -37,6 +37,7 @@
> TT_OverloadedOperator,
> TT_PointerOrReference,
> TT_ConditionalExpr,
> + TT_CtorInitializerColon,
> TT_LineComment,
> TT_BlockComment
> };
> @@ -73,7 +74,6 @@
> }
>
> struct OptimizationParameters {
> - unsigned PenaltyExtraLine;
> unsigned PenaltyIndentLevel;
> };
>
> @@ -83,13 +83,9 @@
> const UnwrappedLine &Line,
> const std::vector<TokenAnnotation> &Annotations,
> tooling::Replacements &Replaces, bool
> StructuralError)
> - : Style(Style),
> - SourceMgr(SourceMgr),
> - Line(Line),
> - Annotations(Annotations),
> - Replaces(Replaces),
> + : Style(Style), SourceMgr(SourceMgr), Line(Line),
> + Annotations(Annotations), Replaces(Replaces),
> StructuralError(StructuralError) {
> - Parameters.PenaltyExtraLine = 100;
> Parameters.PenaltyIndentLevel = 5;
> }
>
> @@ -100,8 +96,6 @@
> // Initialize state dependent on indent.
> IndentState State;
> State.Column = Indent;
> - State.CtorInitializerOnNewLine = false;
> - State.InCtorInitializer = false;
> State.ConsumedTokens = 0;
> State.Indent.push_back(Indent + 4);
> State.LastSpace.push_back(Indent);
> @@ -110,11 +104,33 @@
> // The first token has already been indented and thus consumed.
> moveStateToNextToken(State);
>
> + // Check whether the UnwrappedLine can be put onto a single line. If
> so,
> + // this is bound to be the optimal solution (by definition) and we
> don't
> + // need to analyze the entire solution space.
> + unsigned Columns = State.Column;
> + bool FitsOnALine = true;
> + for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
> + Columns += (Annotations[i].SpaceRequiredBefore ? 1 : 0) +
> + Line.Tokens[i].Tok.getLength();
> + // A special case for the colon of a constructor initializer as
> this only
> + // needs to be put on a new line if the line needs to be split.
> + if (Columns > Style.ColumnLimit ||
> + (Annotations[i].MustBreakBefore &&
> + Annotations[i].Type !=
> TokenAnnotation::TT_CtorInitializerColon)) {
> + FitsOnALine = false;
> + break;
> + }
> + }
> +
> // Start iterating at 1 as we have correctly formatted of Token #0
> above.
> for (unsigned i = 1, n = Line.Tokens.size(); i != n; ++i) {
> - unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
> - unsigned Break = calcPenalty(State, true, NoBreak);
> - addTokenToState(Break < NoBreak, false, State);
> + if (FitsOnALine) {
> + addTokenToState(false, false, State);
> + } else {
> + unsigned NoBreak = calcPenalty(State, false, UINT_MAX);
> + unsigned Break = calcPenalty(State, true, NoBreak);
> + addTokenToState(Break < NoBreak, false, State);
> + }
> }
> }
>
> @@ -146,9 +162,6 @@
> /// on a level.
> std::vector<unsigned> FirstLessLess;
>
> - bool CtorInitializerOnNewLine;
> - bool InCtorInitializer;
> -
> /// \brief Comparison operator to be able to used \c IndentState in
> \c map.
> bool operator<(const IndentState &Other) const {
> if (Other.ConsumedTokens != ConsumedTokens)
> @@ -212,11 +225,8 @@
>
> State.LastSpace[ParenLevel] = State.Indent[ParenLevel];
> if (Current.Tok.is(tok::colon) &&
> - Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr)
> {
> + Annotations[Index].Type != TokenAnnotation::TT_ConditionalExpr)
> State.Indent[ParenLevel] += 2;
> - State.CtorInitializerOnNewLine = true;
> - State.InCtorInitializer = true;
> - }
> } else {
> unsigned Spaces = Annotations[Index].SpaceRequiredBefore ? 1 : 0;
> if (Annotations[Index].Type == TokenAnnotation::TT_LineComment)
> @@ -228,10 +238,7 @@
> if (Previous.Tok.is(tok::l_paren) ||
> Annotations[Index - 1].Type ==
> TokenAnnotation::TT_TemplateOpener)
> State.Indent[ParenLevel] = State.Column;
> - if (Current.Tok.is(tok::colon)) {
> - State.Indent[ParenLevel] = State.Column + 3;
> - State.InCtorInitializer = true;
> - }
> +
> // Top-level spaces are exempt as that mostly leads to better
> results.
> State.Column += Spaces;
> if (Spaces > 0 && ParenLevel != 0)
> @@ -279,10 +286,8 @@
> "Tried to calculate penalty for splitting after the last
> token");
> const FormatToken &Left = Line.Tokens[Index];
> const FormatToken &Right = Line.Tokens[Index + 1];
> - if (Left.Tok.is(tok::semi))
> + if (Left.Tok.is(tok::semi) || Left.Tok.is(tok::comma))
> return 0;
> - if (Left.Tok.is(tok::comma))
> - return 1;
> if (Left.Tok.is(tok::equal) || Left.Tok.is(tok::l_paren) ||
> Left.Tok.is(tok::pipepipe) || Left.Tok.is(tok::ampamp))
> return 2;
> @@ -313,18 +318,10 @@
> if (NewLine && !Annotations[State.ConsumedTokens].CanBreakBefore)
> return UINT_MAX;
>
> - if (State.ConsumedTokens > 0 && !NewLine &&
> - State.CtorInitializerOnNewLine &&
> - Line.Tokens[State.ConsumedTokens - 1].Tok.is(tok::comma))
> - return UINT_MAX;
> -
> - if (NewLine && State.InCtorInitializer &&
> !State.CtorInitializerOnNewLine)
> - return UINT_MAX;
> -
> unsigned CurrentPenalty = 0;
> if (NewLine) {
> CurrentPenalty += Parameters.PenaltyIndentLevel *
> State.Indent.size() +
> - Parameters.PenaltyExtraLine + splitPenalty(State.ConsumedTokens
> - 1);
> + splitPenalty(State.ConsumedTokens - 1);
> }
>
> addTokenToState(NewLine, true, State);
> @@ -413,9 +410,7 @@
> public:
> TokenAnnotator(const UnwrappedLine &Line, const FormatStyle &Style,
> SourceManager &SourceMgr)
> - : Line(Line),
> - Style(Style),
> - SourceMgr(SourceMgr) {
> + : Line(Line), Style(Style), SourceMgr(SourceMgr) {
> }
>
> /// \brief A parser that gathers additional information about tokens.
> @@ -427,9 +422,7 @@
> public:
> AnnotatingParser(const SmallVector<FormatToken, 16> &Tokens,
> std::vector<TokenAnnotation> &Annotations)
> - : Tokens(Tokens),
> - Annotations(Annotations),
> - Index(0) {
> + : Tokens(Tokens), Annotations(Annotations), Index(0) {
> }
>
> bool parseAngle() {
> @@ -496,6 +489,10 @@
> switch (Tokens[CurrentIndex].Tok.getKind()) {
> case tok::l_paren:
> parseParens();
> + if (Index < Tokens.size() && Tokens[Index].Tok.is(tok::colon)) {
> + Annotations[Index].Type =
> TokenAnnotation::TT_CtorInitializerColon;
> + next();
> + }
> break;
> case tok::l_square:
> parseSquare();
> @@ -557,7 +554,10 @@
> Annotation.CanBreakBefore =
> canBreakBetween(Line.Tokens[i - 1], Line.Tokens[i]);
>
> - if (Line.Tokens[i].Tok.is(tok::colon)) {
> + if (Annotation.Type == TokenAnnotation::TT_CtorInitializerColon) {
> + Annotation.MustBreakBefore = true;
> + Annotation.SpaceRequiredBefore = true;
> + } else if (Line.Tokens[i].Tok.is(tok::colon)) {
> Annotation.SpaceRequiredBefore =
> Line.Tokens[0].Tok.isNot(tok::kw_case) && i != e - 1;
> } else if (Annotations[i - 1].Type ==
> TokenAnnotation::TT_UnaryOperator) {
> @@ -769,9 +769,7 @@
> class LexerBasedFormatTokenSource : public FormatTokenSource {
> public:
> LexerBasedFormatTokenSource(Lexer &Lex, SourceManager &SourceMgr)
> - : GreaterStashed(false),
> - Lex(Lex),
> - SourceMgr(SourceMgr),
> + : GreaterStashed(false), Lex(Lex), SourceMgr(SourceMgr),
> IdentTable(Lex.getLangOpts()) {
> Lex.SetKeepWhitespaceMode(true);
> }
> @@ -831,10 +829,7 @@
> public:
> Formatter(const FormatStyle &Style, Lexer &Lex, SourceManager
> &SourceMgr,
> const std::vector<CharSourceRange> &Ranges)
> - : Style(Style),
> - Lex(Lex),
> - SourceMgr(SourceMgr),
> - Ranges(Ranges),
> + : Style(Style), Lex(Lex), SourceMgr(SourceMgr), Ranges(Ranges),
> StructuralError(false) {
> }
>
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=170457&r1=170456&r2=170457&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Tue Dec 18 15:05:13 2012
> @@ -374,6 +374,41 @@
> " parameter, parameter, parameter)),
> SecondLongCall(parameter));");
> }
>
> +TEST_F(FormatTest, ConstructorInitializers) {
> + verifyFormat("Constructor() : Initializer(FitsOnTheLine) {\n}");
> +
> + verifyFormat(
> + "SomeClass::Constructor()\n"
> + " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa)
> {\n"
> + "}");
> +
> + verifyFormat(
> + "SomeClass::Constructor()\n"
> + " :
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
> + " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {\n"
> + "}");
> +
> + verifyFormat("Constructor()\n"
> + " :
> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
> + "
> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
> + "
> aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
> + " aaaaaaaaaaaaaaaaaaaaaaa() {\n"
> + "}");
> +
> + // Here a line could be saved by splitting the second initializer onto
> two
> + // lines, but that is not desireable.
> + verifyFormat("Constructor()\n"
> + " :
> aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaa),\n"
> + " aaaaaaaaaaa(aaaaaaaaaaa),\n"
> + "
> aaaaaaaaaaaaaaaaaaaaat(aaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
> + "}");
> +
> + verifyGoogleFormat("MyClass::MyClass(int var)\n"
> + " : some_var_(var), // 4 space indent\n"
> + " some_other_var_(var + 1) { // lined up\n"
> + "}");
> +}
> +
> TEST_F(FormatTest, BreaksAsHighAsPossible) {
> verifyFormat(
> "if ((aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
> aaaaaaaaaaaaaaaaaaaaaaaaaa) ||\n"
> @@ -461,13 +496,11 @@
> }
>
> TEST_F(FormatTest, WrapsAtFunctionCallsIfNecessary) {
> - verifyFormat(
> - "LoooooooooooooooooooooooooooooooooooooongObject\n"
> - " .looooooooooooooooooooooooooooooooooooooongFunction();");
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
> + "
> .looooooooooooooooooooooooooooooooooooooongFunction();");
>
> - verifyFormat(
> - "LoooooooooooooooooooooooooooooooooooooongObject\n"
> - " ->looooooooooooooooooooooooooooooooooooooongFunction();");
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooongObject\n"
> + "
> ->looooooooooooooooooooooooooooooooooooooongFunction();");
>
> verifyFormat(
>
> "LooooooooooooooooooooooooooooooooongObject->shortFunction(Parameter1,\n"
> @@ -485,10 +518,9 @@
> "function(LoooooooooooooooooooooooooooooooooooongObject\n"
> "
> ->loooooooooooooooooooooooooooooooooooooooongFunction());");
>
> - verifyFormat(
> - "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"
> - " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
> - "}");
> + verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa)
> ||\n"
> + " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n"
> + "}");
> }
>
> TEST_F(FormatTest, UnderstandsTemplateParameters) {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121218/15c89bbd/attachment.html>
More information about the cfe-commits
mailing list