<div dir="ltr">Fixed in <span style="color:rgb(0,0,0);white-space:pre-wrap">r173043. Sorry for the trouble..</span></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jan 20, 2013 at 4:07 PM, NAKAMURA Takumi <span dir="ltr"><<a href="mailto:geek4civic@gmail.com" target="_blank">geek4civic@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Daniel,<br>
<br>
It causes an error in valgrind on<br>
FormatTest.DoesNotTouchUnwrappedLinesWithErrors.<br>
<a href="http://lab.llvm.org:8011/builders/clang-x86_64-linux-vg/builds/643" target="_blank">http://lab.llvm.org:8011/builders/clang-x86_64-linux-vg/builds/643</a><br>
<br>
In nextTwoLinesFitInto(),<br>
(I + 2)->Last->TotalLength is uninitialized.<br>
<br>
Please take a look.<br>
<br>
...Takumi<br>
<br>
2013/1/16 Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>>:<br>
<div class="HOEnZb"><div class="h5">> Author: djasper<br>
> Date: Wed Jan 16 04:41:46 2013<br>
> New Revision: 172604<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172604&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=172604&view=rev</a><br>
> Log:<br>
> Calculate the total length of a line up to each token up front.<br>
><br>
> This makes the tedious fitsIntoLimit() method unnecessary and I can<br>
> replace one hack (constructor initializers) by a slightly better hack.<br>
><br>
> Furthermore, this will enable calculating whether a certain part of a<br>
> line fits into the limit for future modifications.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Format/Format.cpp<br>
> cfe/trunk/lib/Format/UnwrappedLineParser.h<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=172604&r1=172603&r2=172604&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=172604&r1=172603&r2=172604&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Wed Jan 16 04:41:46 2013<br>
> @@ -86,6 +86,9 @@<br>
><br>
> bool ClosesTemplateDeclaration;<br>
><br>
> + /// \brief The total length of the line up to and including this token.<br>
> + unsigned TotalLength;<br>
> +<br>
> std::vector<AnnotatedToken> Children;<br>
> AnnotatedToken *Parent;<br>
> };<br>
> @@ -207,31 +210,6 @@<br>
> NewLineText + std::string(Spaces, ' ')));<br>
> }<br>
><br>
> -/// \brief Calculates whether the (remaining) \c AnnotatedLine starting with<br>
> -/// \p RootToken fits into \p Limit columns on a single line.<br>
> -///<br>
> -/// If true, sets \p Length to the required length.<br>
> -static bool fitsIntoLimit(const AnnotatedToken &RootToken, unsigned Limit,<br>
> - unsigned *Length = 0) {<br>
> - unsigned Columns = RootToken.FormatTok.TokenLength;<br>
> - if (Columns > Limit) return false;<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>
> - // FIXME: Remove this hack.<br>
> - return false;<br>
> - }<br>
> - }<br>
> - if (Length != 0)<br>
> - *Length = Columns;<br>
> - return true;<br>
> -}<br>
> -<br>
> /// \brief Returns if a token is an Objective-C selector name.<br>
> ///<br>
> /// For example, "bar" is a selector name in [foo bar:(4 + 5)].<br>
> @@ -245,11 +223,10 @@<br>
> public:<br>
> UnwrappedLineFormatter(const FormatStyle &Style, SourceManager &SourceMgr,<br>
> const AnnotatedLine &Line, unsigned FirstIndent,<br>
> - bool FitsOnALine, const AnnotatedToken &RootToken,<br>
> + const AnnotatedToken &RootToken,<br>
> tooling::Replacements &Replaces, bool StructuralError)<br>
> : Style(Style), SourceMgr(SourceMgr), Line(Line),<br>
> - FirstIndent(FirstIndent), FitsOnALine(FitsOnALine),<br>
> - RootToken(RootToken), Replaces(Replaces) {<br>
> + FirstIndent(FirstIndent), RootToken(RootToken), Replaces(Replaces) {<br>
> Parameters.PenaltyIndentLevel = 15;<br>
> Parameters.PenaltyLevelDecrease = 30;<br>
> Parameters.PenaltyExcessCharacter = 1000000;<br>
> @@ -280,7 +257,7 @@<br>
> // cannot continue an top-level implicit string literal on the next<br>
> // line.<br>
> return 0;<br>
> - if (FitsOnALine) {<br>
> + if (Line.Last->TotalLength <= getColumnLimit() - FirstIndent) {<br>
> addTokenToState(false, false, State);<br>
> } else {<br>
> unsigned NoBreak = calcPenalty(State, false, UINT_MAX);<br>
> @@ -289,8 +266,7 @@<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>
> + Line.Last->TotalLength > getColumnLimit() - State.Column - 1)<br>
> State.Stack.back().BreakAfterComma = true;<br>
> }<br>
> }<br>
> @@ -607,6 +583,8 @@<br>
> State.NextToken->Type != TT_LineComment &&<br>
> State.Stack.back().BreakAfterComma)<br>
> return UINT_MAX;<br>
> + if (!NewLine && State.NextToken->Type == TT_CtorInitializerColon)<br>
> + return UINT_MAX;<br>
><br>
> unsigned CurrentPenalty = 0;<br>
> if (NewLine) {<br>
> @@ -658,7 +636,6 @@<br>
> SourceManager &SourceMgr;<br>
> const AnnotatedLine &Line;<br>
> const unsigned FirstIndent;<br>
> - const bool FitsOnALine;<br>
> const AnnotatedToken &RootToken;<br>
> tooling::Replacements &Replaces;<br>
><br>
> @@ -974,8 +951,7 @@<br>
> } else {<br>
> if (Current.Type == TT_LineComment) {<br>
> Current.MustBreakBefore = Current.FormatTok.NewlinesBefore > 0;<br>
> - } else if (Current.Type == TT_CtorInitializerColon ||<br>
> - Current.Parent->Type == TT_LineComment ||<br>
> + } else if (Current.Parent->Type == TT_LineComment ||<br>
> (Current.is(tok::string_literal) &&<br>
> Current.Parent->is(tok::string_literal))) {<br>
> Current.MustBreakBefore = true;<br>
> @@ -984,6 +960,12 @@<br>
> }<br>
> }<br>
> Current.CanBreakBefore = Current.MustBreakBefore || canBreakBefore(Current);<br>
> + if (Current.MustBreakBefore)<br>
> + Current.TotalLength = Current.Parent->TotalLength + Style.ColumnLimit;<br>
> + else<br>
> + Current.TotalLength = Current.Parent->TotalLength +<br>
> + Current.FormatTok.TokenLength +<br>
> + (Current.SpaceRequiredBefore ? 1 : 0);<br>
> if (!Current.Children.empty())<br>
> calculateExtraInformation(Current.Children[0]);<br>
> }<br>
> @@ -1007,6 +989,7 @@<br>
> Line.First.MustBreakBefore = Line.First.FormatTok.MustBreakBefore;<br>
> Line.First.CanBreakBefore = Line.First.MustBreakBefore;<br>
><br>
> + Line.First.TotalLength = Line.First.FormatTok.TokenLength;<br>
> if (!Line.First.Children.empty())<br>
> calculateExtraInformation(Line.First.Children[0]);<br>
> }<br>
> @@ -1450,9 +1433,9 @@<br>
> unsigned Indent = formatFirstToken(TheLine.First, TheLine.Level,<br>
> TheLine.InPPDirective,<br>
> PreviousEndOfLineColumn);<br>
> - bool FitsOnALine = tryFitMultipleLinesInOne(Indent, I, E);<br>
> + tryFitMultipleLinesInOne(Indent, I, E);<br>
> UnwrappedLineFormatter Formatter(Style, SourceMgr, TheLine, Indent,<br>
> - FitsOnALine, TheLine.First, Replaces,<br>
> + TheLine.First, Replaces,<br>
> StructuralError);<br>
> PreviousEndOfLineColumn = Formatter.format();<br>
> } else {<br>
> @@ -1477,27 +1460,24 @@<br>
> /// if possible; note that \c I will be incremented when lines are merged.<br>
> ///<br>
> /// Returns whether the resulting \c Line can fit in a single line.<br>
> - bool tryFitMultipleLinesInOne(unsigned Indent,<br>
> + void tryFitMultipleLinesInOne(unsigned Indent,<br>
> std::vector<AnnotatedLine>::iterator &I,<br>
> std::vector<AnnotatedLine>::iterator E) {<br>
> unsigned Limit = Style.ColumnLimit - (I->InPPDirective ? 1 : 0) - Indent;<br>
><br>
> + // We can never merge stuff if there are trailing line comments.<br>
> + if (I->Last->Type == TT_LineComment)<br>
> + return;<br>
> +<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>
> - unsigned Length = 0;<br>
> - if (!fitsIntoLimit(I->First, Limit, &Length))<br>
> - return false;<br>
> -<br>
> - // We can never merge stuff if there are trailing line comments.<br>
> - if (I->Last->Type == TT_LineComment)<br>
> - return true;<br>
> + if (I->Last->TotalLength >= Limit)<br>
> + return;<br>
> + Limit -= I->Last->TotalLength + 1; // One space.<br>
><br>
> - if (Limit == Length)<br>
> - return true; // Couldn't fit a space.<br>
> - Limit -= Length + 1; // One space.<br>
> if (I + 1 == E)<br>
> - return true;<br>
> + return;<br>
><br>
> if (I->Last->is(tok::l_brace)) {<br>
> tryMergeSimpleBlock(I, E, Limit);<br>
> @@ -1507,7 +1487,7 @@<br>
> I->First.FormatTok.IsFirst)) {<br>
> tryMergeSimplePPDirective(I, E, Limit);<br>
> }<br>
> - return true;<br>
> + return;<br>
> }<br>
><br>
> void tryMergeSimplePPDirective(std::vector<AnnotatedLine>::iterator &I,<br>
> @@ -1519,7 +1499,8 @@<br>
> if (I + 2 != E && (I + 2)->InPPDirective &&<br>
> !(I + 2)->First.FormatTok.HasUnescapedNewline)<br>
> return;<br>
> - if (!fitsIntoLimit((I + 1)->First, Limit)) return;<br>
> + if ((I + 1)->Last->TotalLength > Limit)<br>
> + return;<br>
> join(Line, *(++I));<br>
> }<br>
><br>
> @@ -1531,7 +1512,7 @@<br>
> AnnotatedLine &Line = *I;<br>
> if (Line.Last->isNot(tok::r_paren))<br>
> return;<br>
> - if (!fitsIntoLimit((I + 1)->First, Limit))<br>
> + if ((I + 1)->Last->TotalLength > Limit)<br>
> return;<br>
> if ((I + 1)->First.is(tok::kw_if) || (I + 1)->First.Type == TT_LineComment)<br>
> return;<br>
> @@ -1594,12 +1575,7 @@<br>
><br>
> bool nextTwoLinesFitInto(std::vector<AnnotatedLine>::iterator I,<br>
> unsigned Limit) {<br>
> - unsigned LengthLine1 = 0;<br>
> - unsigned LengthLine2 = 0;<br>
> - if (!fitsIntoLimit((I + 1)->First, Limit, &LengthLine1) ||<br>
> - !fitsIntoLimit((I + 2)->First, Limit, &LengthLine2))<br>
> - return false;<br>
> - return LengthLine1 + LengthLine2 + 1 <= Limit; // One space.<br>
> + return (I + 1)->Last->TotalLength + 1 + (I + 2)->Last->TotalLength <= Limit;<br>
> }<br>
><br>
> void join(AnnotatedLine &A, const AnnotatedLine &B) {<br>
><br>
> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=172604&r1=172603&r2=172604&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=172604&r1=172603&r2=172604&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Wed Jan 16 04:41:46 2013<br>
> @@ -88,6 +88,7 @@<br>
> UnwrappedLine() : Level(0), InPPDirective(false) {<br>
> }<br>
><br>
> + // FIXME: Don't use std::list here.<br>
> /// \brief The \c Tokens comprising this \c UnwrappedLine.<br>
> std::list<FormatToken> Tokens;<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>