On Fri, Jan 11, 2013 at 3:37 AM, Daniel Jasper <[email protected]> wrote: > Author: djasper > Date: Fri Jan 11 05:37:55 2013 > New Revision: 172196 > > URL: http://llvm.org/viewvc/llvm-project?rev=172196&view=rev > Log: > Improved formatting of constructor initializers > > Added option to put each constructor initializer on its own line > if not all initializers fit on a single line. Enabling this for > Google style now as the style guide (arguable) suggests it. Not > sure whether we also want it for LLVM. > > Modified: > cfe/trunk/include/clang/Format/Format.h > cfe/trunk/lib/Format/Format.cpp > cfe/trunk/unittests/Format/FormatTest.cpp > > Modified: cfe/trunk/include/clang/Format/Format.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Format/Format.h?rev=172196&r1=172195&r2=172196&view=diff > ============================================================================== > --- cfe/trunk/include/clang/Format/Format.h (original) > +++ cfe/trunk/include/clang/Format/Format.h Fri Jan 11 05:37:55 2013 > @@ -56,6 +56,10 @@ > /// \brief The number of spaces to before trailing line comments. > unsigned SpacesBeforeTrailingComments; > > + /// \brief If the constructor initializers don't fit on a line, put each > + /// initializer on its own line. > + bool ConstructorInitializerAllOnOneLineOrOnePerLine;
I don't think this is a great name for a bool, since it's not clear which behavior is indicated by 'true'. Maybe 'ConstructorInitializerListOnePerLine' or something like that instead? > + > /// \brief Add a space in front of an Objective-C protocol list, i.e. use > /// Foo <Protocol> instead of Foo<Protocol>. > bool ObjCSpaceBeforeProtocolList; > > Modified: cfe/trunk/lib/Format/Format.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=172196&r1=172195&r2=172196&view=diff > ============================================================================== > --- cfe/trunk/lib/Format/Format.cpp (original) > +++ cfe/trunk/lib/Format/Format.cpp Fri Jan 11 05:37:55 2013 > @@ -105,6 +105,7 @@ > LLVMStyle.SplitTemplateClosingGreater = true; > LLVMStyle.IndentCaseLabels = false; > LLVMStyle.SpacesBeforeTrailingComments = 1; > + LLVMStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = false; > LLVMStyle.ObjCSpaceBeforeProtocolList = true; > LLVMStyle.ObjCSpaceBeforeReturnType = true; > return LLVMStyle; > @@ -119,6 +120,7 @@ > GoogleStyle.SplitTemplateClosingGreater = false; > GoogleStyle.IndentCaseLabels = true; > GoogleStyle.SpacesBeforeTrailingComments = 2; > + GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true; > GoogleStyle.ObjCSpaceBeforeProtocolList = false; > GoogleStyle.ObjCSpaceBeforeReturnType = false; > return GoogleStyle; > @@ -165,6 +167,26 @@ > NewLineText + std::string(Spaces, ' > '))); > } > > +/// \brief Checks whether the (remaining) \c UnwrappedLine starting with > +/// \p RootToken fits into \p Limit columns. > +bool fitsIntoLimit(const AnnotatedToken &RootToken, unsigned Limit) { > + unsigned Columns = RootToken.FormatTok.TokenLength; > + bool FitsOnALine = true; > + const AnnotatedToken *Tok = &RootToken; > + while (!Tok->Children.empty()) { > + Tok = &Tok->Children[0]; > + Columns += (Tok->SpaceRequiredBefore ? 1 : 0) + > Tok->FormatTok.TokenLength; > + // 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 > Limit || > + (Tok->MustBreakBefore && Tok->Type != TT_CtorInitializerColon)) { > + FitsOnALine = false; > + break; > + } > + }; > + return FitsOnALine; > +} > + > class UnwrappedLineFormatter { > public: > UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr, > @@ -190,7 +212,7 @@ > LineState State; > State.Column = FirstIndent; > State.NextToken = &RootToken; > - State.Stack.push_back(ParenState(FirstIndent + 4, FirstIndent, 0, > false)); > + State.Stack.push_back(ParenState(FirstIndent + 4, FirstIndent)); > State.ForLoopVariablePos = 0; > State.LineContainsContinuedForLoopSection = false; > State.StartOfLineLevel = 1; > @@ -206,6 +228,13 @@ > unsigned NoBreak = calcPenalty(State, false, UINT_MAX); > unsigned Break = calcPenalty(State, true, NoBreak); > addTokenToState(Break < NoBreak, false, State); > + if (State.NextToken != NULL && > + State.NextToken->Parent->Type == TT_CtorInitializerColon) { > + if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine && > + !fitsIntoLimit(*State.NextToken, > + getColumnLimit() - State.Column - 1)) > + State.Stack.back().BreakAfterComma = true; > + } > } > } > return State.Column; > @@ -213,10 +242,9 @@ > > private: > struct ParenState { > - ParenState(unsigned Indent, unsigned LastSpace, unsigned FirstLessLess, > - bool BreakBeforeClosingBrace) > - : Indent(Indent), LastSpace(LastSpace), FirstLessLess(FirstLessLess), > - BreakBeforeClosingBrace(BreakBeforeClosingBrace) {} > + ParenState(unsigned Indent, unsigned LastSpace) > + : Indent(Indent), LastSpace(LastSpace), FirstLessLess(0), > + BreakBeforeClosingBrace(false), BreakAfterComma(false) {} > > /// \brief The position to which a specific parenthesis level needs to be > /// indented. > @@ -242,6 +270,8 @@ > /// was a newline after the beginning left brace. > bool BreakBeforeClosingBrace; > > + bool BreakAfterComma; > + > bool operator<(const ParenState &Other) const { > if (Indent != Other.Indent) > return Indent < Other.Indent; > @@ -249,7 +279,9 @@ > return LastSpace < Other.LastSpace; > if (FirstLessLess != Other.FirstLessLess) > return FirstLessLess < Other.FirstLessLess; > - return BreakBeforeClosingBrace; > + if (BreakBeforeClosingBrace != Other.BreakBeforeClosingBrace) > + return BreakBeforeClosingBrace; > + return BreakAfterComma; > } > }; > > @@ -416,7 +448,7 @@ > NewIndent = 4 + State.Stack.back().LastSpace; > } > State.Stack.push_back( > - ParenState(NewIndent, State.Stack.back().LastSpace, 0, false)); > + ParenState(NewIndent, State.Stack.back().LastSpace)); > } > > // If we encounter a closing ), ], } or >, we can remove a level from our > @@ -498,6 +530,10 @@ > if (!NewLine && State.NextToken->Parent->is(tok::semi) && > State.LineContainsContinuedForLoopSection) > return UINT_MAX; > + if (!NewLine && State.NextToken->Parent->is(tok::comma) && > + State.NextToken->Type != TT_LineComment && > + State.Stack.back().BreakAfterComma) > + return UINT_MAX; > > unsigned CurrentPenalty = 0; > if (NewLine) { > @@ -1250,8 +1286,12 @@ > unsigned Indent = formatFirstToken(Annotator.getRootToken(), > TheLine.Level, > TheLine.InPPDirective, > PreviousEndOfLineColumn); > - bool FitsOnALine = fitsOnALine(Annotator.getRootToken(), Indent, > - TheLine.InPPDirective); > + unsigned Limit = Style.ColumnLimit - (TheLine.InPPDirective ? 1 : 0) > - > + Indent; > + // 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. > + bool FitsOnALine = fitsIntoLimit(Annotator.getRootToken(), Limit); > UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent, > FitsOnALine, > Annotator.getLineType(), > Annotator.getRootToken(), Replaces, > @@ -1300,29 +1340,6 @@ > UnwrappedLines.push_back(TheLine); > } > > - bool fitsOnALine(const AnnotatedToken &RootToken, unsigned Indent, > - bool InPPDirective) { > - // 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 = Indent + RootToken.FormatTok.TokenLength; > - bool FitsOnALine = true; > - const AnnotatedToken *Tok = &RootToken; > - while (Tok != NULL) { > - Columns += (Tok->SpaceRequiredBefore ? 1 : 0) + > - Tok->FormatTok.TokenLength; > - // 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 - (InPPDirective ? 1 : 0) || > - (Tok->MustBreakBefore && Tok->Type != TT_CtorInitializerColon)) { > - FitsOnALine = false; > - break; > - } > - Tok = Tok->Children.empty() ? NULL : &Tok->Children[0]; > - } > - return FitsOnALine; > - } > - > /// \brief Add a new line and the required indent before the first Token > /// of the \c UnwrappedLine if there was no structural parsing error. > /// Returns the indent level of the \c UnwrappedLine. > > Modified: cfe/trunk/unittests/Format/FormatTest.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=172196&r1=172195&r2=172196&view=diff > ============================================================================== > --- cfe/trunk/unittests/Format/FormatTest.cpp (original) > +++ cfe/trunk/unittests/Format/FormatTest.cpp Fri Jan 11 05:37:55 2013 > @@ -649,6 +649,11 @@ > > TEST_F(FormatTest, ConstructorInitializers) { > verifyFormat("Constructor() : Initializer(FitsOnTheLine) {}"); > + verifyFormat("Constructor() : Inttializer(FitsOnTheLine) {}", > + getLLVMStyleWithColumns(45)); > + verifyFormat("Constructor()\n" > + " : Inttializer(FitsOnTheLine) {}", > + getLLVMStyleWithColumns(44)); > > verifyFormat( > "SomeClass::Constructor()\n" > @@ -656,6 +661,16 @@ > > verifyFormat( > "SomeClass::Constructor()\n" > + " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n" > + " aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}"); > + verifyGoogleFormat( > + "SomeClass::Constructor()\n" > + " : aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n" > + " aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n" > + " aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}"); > + > + verifyFormat( > + "SomeClass::Constructor()\n" > " : > aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n" > " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}"); > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
