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