<div dir="ltr">I'd say eventually yes. But for me it is by far not the most pressing thing right now.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 7:31 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Jan 16, 2013 at 1:10 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
> Author: djasper<br>
> Date: Wed Jan 16 03:10:19 2013<br>
> New Revision: 172602<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172602&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=172602&view=rev</a><br>
> Log:<br>
> Change the datastructure for UnwrappedLines.<br>
><br>
> It was quite convoluted leading to us accidentally introducing O(N^2)<br>
> complexity while copying from UnwrappedLine to AnnotatedLine. We might<br>
> still want to improve the datastructure in AnnotatedLine (most<br>
> importantly not put them in a vector where they need to be copied on<br>
> vector resizing but that will be done as a follow-up.<br>
><br>
> This fixes most of the regression in <a href="http://llvm.org/PR14959" target="_blank">llvm.org/PR14959</a>.<br>
<br>
</div>Thanks! Do you think it makes sense to have an automated perf test for<br>
libFormat?<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> No formatting changes intended.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Format/Format.cpp<br>
> cfe/trunk/lib/Format/UnwrappedLineParser.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=172602&r1=172601&r2=172602&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=172602&r1=172601&r2=172602&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Wed Jan 16 03:10:19 2013<br>
> @@ -92,9 +92,30 @@<br>
><br>
> class AnnotatedLine {<br>
> public:<br>
> - AnnotatedLine(const FormatToken &FormatTok, unsigned Level,<br>
> - bool InPPDirective)<br>
> - : First(FormatTok), Level(Level), InPPDirective(InPPDirective) {}<br>
> + AnnotatedLine(const UnwrappedLine &Line)<br>
> + : First(Line.Tokens.front()), Level(Line.Level),<br>
> + InPPDirective(Line.InPPDirective) {<br>
> + assert(!Line.Tokens.empty());<br>
> + AnnotatedToken *Current = &First;<br>
> + for (std::list<FormatToken>::const_iterator I = ++Line.Tokens.begin(),<br>
> + E = Line.Tokens.end();<br>
> + I != E; ++I) {<br>
> + Current->Children.push_back(*I);<br>
> + Current->Children[0].Parent = Current;<br>
> + Current = &Current->Children[0];<br>
> + }<br>
> + Last = Current;<br>
> + }<br>
> + AnnotatedLine(const AnnotatedLine &Other)<br>
> + : First(Other.First), Type(Other.Type), Level(Other.Level),<br>
> + InPPDirective(Other.InPPDirective) {<br>
> + Last = &First;<br>
> + while (!Last->Children.empty()) {<br>
> + Last->Children[0].Parent = Last;<br>
> + Last = &Last->Children[0];<br>
> + }<br>
> + }<br>
> +<br>
> AnnotatedToken First;<br>
> AnnotatedToken *Last;<br>
><br>
> @@ -945,16 +966,6 @@<br>
> bool ColonIsObjCMethodExpr;<br>
> };<br>
><br>
> - void createAnnotatedTokens(AnnotatedToken &Current) {<br>
> - if (Current.FormatTok.Children.empty()) {<br>
> - Line.Last = &Current;<br>
> - } else {<br>
> - Current.Children.push_back(AnnotatedToken(Current.FormatTok.Children[0]));<br>
> - Current.Children.back().Parent = &Current;<br>
> - createAnnotatedTokens(Current.Children.back());<br>
> - }<br>
> - }<br>
> -<br>
> void calculateExtraInformation(AnnotatedToken &Current) {<br>
> Current.SpaceRequiredBefore = spaceRequiredBefore(Current);<br>
><br>
> @@ -978,9 +989,6 @@<br>
> }<br>
><br>
> void annotate() {<br>
> - Line.Last = &Line.First;<br>
> - createAnnotatedTokens(Line.First);<br>
> -<br>
> AnnotatingParser Parser(Line.First);<br>
> Line.Type = Parser.parseLine();<br>
> if (Line.Type == LT_Invalid)<br>
> @@ -1619,8 +1627,7 @@<br>
> }<br>
><br>
> virtual void consumeUnwrappedLine(const UnwrappedLine &TheLine) {<br>
> - AnnotatedLines.push_back(<br>
> - AnnotatedLine(TheLine.RootToken, TheLine.Level, TheLine.InPPDirective));<br>
> + AnnotatedLines.push_back(AnnotatedLine(TheLine));<br>
> }<br>
><br>
> /// \brief Add a new line and the required indent before the first Token<br>
><br>
> Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=172602&r1=172601&r2=172602&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=172602&r1=172601&r2=172602&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Wed Jan 16 03:10:19 2013<br>
> @@ -81,25 +81,17 @@<br>
> public:<br>
> ScopedLineState(UnwrappedLineParser &Parser) : Parser(Parser) {<br>
> PreBlockLine = Parser.Line.take();<br>
> - Parser.Line.reset(new UnwrappedLine(*PreBlockLine));<br>
> - assert(Parser.LastInCurrentLine == NULL ||<br>
> - Parser.LastInCurrentLine->Children.empty());<br>
> - PreBlockLastToken = Parser.LastInCurrentLine;<br>
> - PreBlockRootTokenInitialized = Parser.RootTokenInitialized;<br>
> - Parser.RootTokenInitialized = false;<br>
> - Parser.LastInCurrentLine = NULL;<br>
> + Parser.Line.reset(new UnwrappedLine());<br>
> + Parser.Line->Level = PreBlockLine->Level;<br>
> + Parser.Line->InPPDirective = PreBlockLine->InPPDirective;<br>
> }<br>
><br>
> ~ScopedLineState() {<br>
> - if (Parser.RootTokenInitialized) {<br>
> + if (!Parser.Line->Tokens.empty()) {<br>
> Parser.addUnwrappedLine();<br>
> }<br>
> - assert(!Parser.RootTokenInitialized);<br>
> + assert(Parser.Line->Tokens.empty());<br>
> Parser.Line.reset(PreBlockLine);<br>
> - Parser.RootTokenInitialized = PreBlockRootTokenInitialized;<br>
> - Parser.LastInCurrentLine = PreBlockLastToken;<br>
> - assert(Parser.LastInCurrentLine == NULL ||<br>
> - Parser.LastInCurrentLine->Children.empty());<br>
> Parser.MustBreakBeforeNextToken = true;<br>
> }<br>
><br>
> @@ -107,15 +99,12 @@<br>
> UnwrappedLineParser &Parser;<br>
><br>
> UnwrappedLine *PreBlockLine;<br>
> - FormatToken* PreBlockLastToken;<br>
> - bool PreBlockRootTokenInitialized;<br>
> };<br>
><br>
> UnwrappedLineParser::UnwrappedLineParser(<br>
> clang::DiagnosticsEngine &Diag, const FormatStyle &Style,<br>
> FormatTokenSource &Tokens, UnwrappedLineConsumer &Callback)<br>
> - : Line(new UnwrappedLine), RootTokenInitialized(false),<br>
> - LastInCurrentLine(NULL), MustBreakBeforeNextToken(false), Diag(Diag),<br>
> + : Line(new UnwrappedLine), MustBreakBeforeNextToken(false), Diag(Diag),<br>
> Style(Style), Tokens(&Tokens), Callback(Callback) {<br>
> }<br>
><br>
> @@ -659,7 +648,7 @@<br>
> }<br>
><br>
> void UnwrappedLineParser::addUnwrappedLine() {<br>
> - if (!RootTokenInitialized)<br>
> + if (Line->Tokens.empty())<br>
> return;<br>
> // Consume trailing comments.<br>
> while (!eof() && FormatTok.NewlinesBefore == 0 &&<br>
> @@ -667,17 +656,17 @@<br>
> nextToken();<br>
> }<br>
> #ifdef UNWRAPPED_LINE_PARSER_DEBUG_OUTPUT<br>
> - FormatToken* NextToken = &Line->RootToken;<br>
> llvm::errs() << "Line: ";<br>
> - while (NextToken) {<br>
> - llvm::errs() << NextToken->Tok.getName() << " ";<br>
> - NextToken = NextToken->Children.empty() ? NULL : &NextToken->Children[0];<br>
> + for (std::list<FormatToken>::iterator I = Line->Tokens.begin(),<br>
> + E = Line->Tokens.end();<br>
> + I != E; ++I) {<br>
> + llvm::errs() << I->Tok.getName() << " ";<br>
> +<br>
> }<br>
> llvm::errs() << "\n";<br>
> #endif<br>
> Callback.consumeUnwrappedLine(*Line);<br>
> - RootTokenInitialized = false;<br>
> - LastInCurrentLine = NULL;<br>
> + Line->Tokens.clear();<br>
> }<br>
><br>
> bool UnwrappedLineParser::eof() const {<br>
> @@ -687,17 +676,9 @@<br>
> void UnwrappedLineParser::nextToken() {<br>
> if (eof())<br>
> return;<br>
> - if (RootTokenInitialized) {<br>
> - assert(LastInCurrentLine->Children.empty());<br>
> - LastInCurrentLine->Children.push_back(FormatTok);<br>
> - LastInCurrentLine = &LastInCurrentLine->Children.back();<br>
> - } else {<br>
> - Line->RootToken = FormatTok;<br>
> - RootTokenInitialized = true;<br>
> - LastInCurrentLine = &Line->RootToken;<br>
> - }<br>
> + Line->Tokens.push_back(FormatTok);<br>
> if (MustBreakBeforeNextToken) {<br>
> - LastInCurrentLine->MustBreakBefore = true;<br>
> + Line->Tokens.back().MustBreakBefore = true;<br>
> MustBreakBeforeNextToken = false;<br>
> }<br>
> readToken();<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=172602&r1=172601&r2=172602&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.h?rev=172602&r1=172601&r2=172602&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/UnwrappedLineParser.h (original)<br>
> +++ cfe/trunk/lib/Format/UnwrappedLineParser.h Wed Jan 16 03:10:19 2013<br>
> @@ -24,7 +24,7 @@<br>
> #include "clang/Format/Format.h"<br>
> #include "clang/Lex/Lexer.h"<br>
><br>
> -#include <vector><br>
> +#include <list><br>
><br>
> namespace clang {<br>
><br>
> @@ -76,11 +76,6 @@<br>
> /// This happens for example when a preprocessor directive ended directly<br>
> /// before the token.<br>
> bool MustBreakBefore;<br>
> -<br>
> - // FIXME: We currently assume that there is exactly one token in this vector<br>
> - // except for the very last token that does not have any children.<br>
> - /// \brief All tokens that logically follow this token.<br>
> - std::vector<FormatToken> Children;<br>
> };<br>
><br>
> /// \brief An unwrapped line is a sequence of \c Token, that we would like to<br>
> @@ -93,8 +88,8 @@<br>
> UnwrappedLine() : Level(0), InPPDirective(false) {<br>
> }<br>
><br>
> - /// \brief The \c Token comprising this \c UnwrappedLine.<br>
> - FormatToken RootToken;<br>
> + /// \brief The \c Tokens comprising this \c UnwrappedLine.<br>
> + std::list<FormatToken> Tokens;<br>
><br>
> /// \brief The indent level of the \c UnwrappedLine.<br>
> unsigned Level;<br>
> @@ -160,8 +155,6 @@<br>
> // subtracted from beyond 0. Introduce a method to subtract from Line.Level<br>
> // and use that everywhere in the Parser.<br>
> OwningPtr<UnwrappedLine> Line;<br>
> - bool RootTokenInitialized;<br>
> - FormatToken *LastInCurrentLine;<br>
> FormatToken FormatTok;<br>
> bool MustBreakBeforeNextToken;<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>