<div dir="ltr">Sorry, I did not know about that MSVC bug. Workaround submitted in r174169.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Feb 1, 2013 at 12:16 PM, Timur Iskhodzhanov <span dir="ltr"><<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br>
<br>
I'm afraid your change breaks the Win build:<br>
<br>
Format.cpp<br>
C:\Program Files (x86)\Microsoft Visual Studio<br>
10.0\VC\include\utility(163): error C2440: 'initializing' : cannot<br>
convert from 'int' to 'const<br>
clang::format::UnwrappedLineFormatter::LineState *'<br>
[tools\clang\lib\Format\clangFormat.vcxproj]<br>
Conversion from integral type to pointer type requires<br>
reinterpret_cast, C-style cast or function-style cast<br>
C:\Program Files (x86)\Microsoft Visual Studio<br>
10.0\VC\include\utility(247) : see reference to function template<br>
instantiation 'std::_Pair_base<_Ty1,_Ty2>::_Pair_base<_Ty,int>(_Other1<br>
&&,_Other2 &&)' being compiled<br>
with<br>
[<br>
_Ty1=bool,<br>
_Ty2=const clang::format::UnwrappedLineFormatter::LineState *,<br>
_Ty=bool,<br>
_Other1=bool,<br>
_Other2=int<br>
]<br>
..\..\..\..\..\llvm\tools\clang\lib\Format\Format.cpp(598) :<br>
see reference to function template instantiation<br>
'std::pair<_Ty1,_Ty2>::pair<bool,int>(_Other1 &&,_Other2 &&)' being<br>
compiled<br>
with<br>
[<br>
_Ty1=bool,<br>
_Ty2=const clang::format::UnwrappedLineFormatter::LineState *,<br>
_Other1=bool,<br>
_Other2=int<br>
]<br>
<br>
C:\Program Files (x86)\Microsoft Visual Studio<br>
10.0\VC\include\utility(163): error C2439:<br>
'std::_Pair_base<_Ty1,_Ty2>::second' : member could not be initialized<br>
[tools\clang\lib\Format\clangFormat.vcxproj]<br>
with<br>
[<br>
_Ty1=bool,<br>
_Ty2=const clang::format::UnwrappedLineFormatter::LineState *<br>
]<br>
C:\Program Files (x86)\Microsoft Visual Studio<br>
10.0\VC\include\utility(167) : see declaration of<br>
'std::_Pair_base<_Ty1,_Ty2>::second'<br>
with<br>
[<br>
_Ty1=bool,<br>
_Ty2=const clang::format::UnwrappedLineFormatter::LineState *<br>
]<br>
<br>
--<br>
Timur<br>
<br>
2013/2/1 Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>>:<br>
<div><div class="h5">> Author: djasper<br>
> Date: Fri Feb 1 05:00:45 2013<br>
> New Revision: 174166<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=174166&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=174166&view=rev</a><br>
> Log:<br>
> Revamp of the basic layouting algorithm in clang-format.<br>
><br>
> In order to end up with good solutions, clang-format needs to try<br>
> "all" combinations of line breaks, evaluate them and select the<br>
> best one. Before, we have done this using a DFS with memoization<br>
> and cut-off conditions. However, this approach is very limited<br>
> as shown by the huge static initializer in the attachment of<br>
> <a href="http://llvm.org/PR14959" target="_blank">llvm.org/PR14959</a>.<br>
><br>
> Instead, this new implementation uses a variant of Dijkstra's<br>
> algorithm to do a prioritized BFS over the solution space.<br>
><br>
> Some numbers:<br>
> lib/Format/TokenAnnotator.cpp: 1.5s -> 0.15s<br>
> Attachment of PR14959: 10min+ (didn't finish) -> 10s<br>
><br>
> No functional changes intended.<br>
><br>
> Modified:<br>
> cfe/trunk/lib/Format/Format.cpp<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=174166&r1=174165&r2=174166&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=174166&r1=174165&r2=174166&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Fri Feb 1 05:00:45 2013<br>
> @@ -241,44 +241,16 @@ public:<br>
> // The first token has already been indented and thus consumed.<br>
> moveStateToNextToken(State);<br>
><br>
> - // Start iterating at 1 as we have correctly formatted of Token #0 above.<br>
> - while (State.NextToken != NULL) {<br>
> - if (State.NextToken->Type == TT_ImplicitStringLiteral) {<br>
> - // Calculating the column is important for aligning trailing comments.<br>
> - // FIXME: This does not seem to happen in conjunction with escaped<br>
> - // newlines. If it does, fix!<br>
> - State.Column += State.NextToken->FormatTok.WhiteSpaceLength +<br>
> - State.NextToken->FormatTok.TokenLength;<br>
> - State.NextToken = State.NextToken->Children.empty()<br>
> - ? NULL : &State.NextToken->Children[0];<br>
> - } else if (Line.Last->TotalLength <= getColumnLimit() - FirstIndent) {<br>
> + // If everything fits on a single line, just put it there.<br>
> + if (Line.Last->TotalLength <= getColumnLimit() - FirstIndent) {<br>
> + while (State.NextToken != NULL) {<br>
> addTokenToState(false, false, State);<br>
> - } else {<br>
> - unsigned NoBreak = calcPenalty(State, false, UINT_MAX);<br>
> - unsigned Break = calcPenalty(State, true, NoBreak);<br>
> - DEBUG({<br>
> - if (Break < NoBreak)<br>
> - llvm::errs() << "\n";<br>
> - else<br>
> - llvm::errs() << " ";<br>
> - llvm::errs() << "<";<br>
> - DebugPenalty(Break, Break < NoBreak);<br>
> - llvm::errs() << "/";<br>
> - DebugPenalty(NoBreak, !(Break < NoBreak));<br>
> - llvm::errs() << "> ";<br>
> - DebugTokenState(*State.NextToken);<br>
> - });<br>
> - addTokenToState(Break < NoBreak, false, State);<br>
> - if (State.NextToken != NULL &&<br>
> - State.NextToken->Parent->Type == TT_CtorInitializerColon) {<br>
> - if (Style.ConstructorInitializerAllOnOneLineOrOnePerLine &&<br>
> - Line.Last->TotalLength > getColumnLimit() - State.Column - 1)<br>
> - State.Stack.back().BreakAfterComma = true;<br>
> - }<br>
> }<br>
> + return State.Column;<br>
> }<br>
> - DEBUG(llvm::errs() << "\n");<br>
> - return State.Column;<br>
> +<br>
> + // Find best solution in solution space.<br>
> + return analyzeSolutionSpace(State);<br>
> }<br>
><br>
> private:<br>
> @@ -289,15 +261,6 @@ private:<br>
> llvm::errs();<br>
> }<br>
><br>
> - void DebugPenalty(unsigned Penalty, bool Winner) {<br>
> - llvm::errs().changeColor(Winner ? raw_ostream::GREEN : raw_ostream::RED);<br>
> - if (Penalty == UINT_MAX)<br>
> - llvm::errs() << "MAX";<br>
> - else<br>
> - llvm::errs() << Penalty;<br>
> - llvm::errs().resetColor();<br>
> - }<br>
> -<br>
> struct ParenState {<br>
> ParenState(unsigned Indent, unsigned LastSpace, bool AvoidBinPacking)<br>
> : Indent(Indent), LastSpace(LastSpace), AssignmentColumn(0),<br>
> @@ -421,6 +384,16 @@ private:<br>
> assert(State.Stack.size());<br>
> unsigned ParenLevel = State.Stack.size() - 1;<br>
><br>
> + if (Current.Type == TT_ImplicitStringLiteral) {<br>
> + State.Column += State.NextToken->FormatTok.WhiteSpaceLength +<br>
> + State.NextToken->FormatTok.TokenLength;<br>
> + if (State.NextToken->Children.empty())<br>
> + State.NextToken = NULL;<br>
> + else<br>
> + State.NextToken = &State.NextToken->Children[0];<br>
> + return;<br>
> + }<br>
> +<br>
> if (Newline) {<br>
> unsigned WhitespaceStartColumn = State.Column;<br>
> if (Current.is(tok::r_brace)) {<br>
> @@ -604,87 +577,123 @@ private:<br>
> return Style.ColumnLimit - (Line.InPPDirective ? 1 : 0);<br>
> }<br>
><br>
> - /// \brief Calculate the number of lines needed to format the remaining part<br>
> - /// of the unwrapped line.<br>
> - ///<br>
> - /// Assumes the formatting so far has led to<br>
> - /// the \c LineSta \p State. If \p NewLine is set, a new line will be<br>
> - /// added after the previous token.<br>
> + /// \brief An edge in the solution space starting from the \c LineState and<br>
> + /// inserting a newline dependent on the \c bool.<br>
> + typedef std::pair<bool, const LineState *> Edge;<br>
> +<br>
> + /// \brief An item in the prioritized BFS search queue. The \c LineState was<br>
> + /// reached using the \c Edge.<br>
> + typedef std::pair<LineState, Edge> QueueItem;<br>
> +<br>
> + /// \brief Analyze the entire solution space starting from \p InitialState.<br>
> ///<br>
> - /// \param StopAt is used for optimization. If we can determine that we'll<br>
> - /// definitely need at least \p StopAt additional lines, we already know of a<br>
> - /// better solution.<br>
> - unsigned calcPenalty(LineState State, bool NewLine, unsigned StopAt) {<br>
> - // We are at the end of the unwrapped line, so we don't need any more lines.<br>
> - if (State.NextToken == NULL)<br>
> + /// This implements a variant of Dijkstra's algorithm on the graph that spans<br>
> + /// the solution space (\c LineStates are the nodes). The algorithm tries to<br>
> + /// find the shortest path (the one with lowest penalty) from \p InitialState<br>
> + /// to a state where all tokens are placed.<br>
> + unsigned analyzeSolutionSpace(const LineState &InitialState) {<br>
> + // Insert start element into queue.<br>
> + std::multimap<unsigned, QueueItem> Queue;<br>
> + Queue.insert(std::pair<unsigned, QueueItem>(<br>
> + 0, QueueItem(InitialState, Edge(false, NULL))));<br>
> + std::map<LineState, Edge> Seen;<br>
> +<br>
> + // While not empty, take first element and follow edges.<br>
> + while (!Queue.empty()) {<br>
> + unsigned Penalty = Queue.begin()->first;<br>
> + QueueItem Item = Queue.begin()->second;<br>
> + if (Item.first.NextToken == NULL)<br>
> + break;<br>
> + Queue.erase(Queue.begin());<br>
> +<br>
> + if (Seen.find(Item.first) != Seen.end())<br>
> + continue; // State already examined with lower penalty.<br>
> +<br>
> + const LineState &SavedState = Seen.insert(std::pair<LineState, Edge>(<br>
> + Item.first,<br>
> + Edge(Item.second.first, Item.second.second))).first->first;<br>
> +<br>
> + addNextStateToQueue(SavedState, Penalty, /*NewLine=*/ false, Queue);<br>
> + addNextStateToQueue(SavedState, Penalty, /*NewLine=*/ true, Queue);<br>
> + }<br>
> +<br>
> + if (Queue.empty())<br>
> + // We were unable to find a solution, do nothing.<br>
> + // FIXME: Add diagnostic?<br>
> return 0;<br>
><br>
> - if (!NewLine && State.NextToken->MustBreakBefore)<br>
> - return UINT_MAX;<br>
> - if (NewLine && !State.NextToken->CanBreakBefore &&<br>
> - !(State.NextToken->is(tok::r_brace) &&<br>
> - State.Stack.back().BreakBeforeClosingBrace))<br>
> - return UINT_MAX;<br>
> - if (!NewLine && State.NextToken->is(tok::r_brace) &&<br>
> - State.Stack.back().BreakBeforeClosingBrace)<br>
> - return UINT_MAX;<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>
> - // Trying to insert a parameter on a new line if there are already more than<br>
> - // one parameter on the current line is bin packing.<br>
> - if (NewLine && State.NextToken->Parent->is(tok::comma) &&<br>
> - State.Stack.back().HasMultiParameterLine &&<br>
> - State.Stack.back().AvoidBinPacking)<br>
> - return UINT_MAX;<br>
> - if (!NewLine && (State.NextToken->Type == TT_CtorInitializerColon ||<br>
> - (State.NextToken->Parent->ClosesTemplateDeclaration &&<br>
> - State.Stack.size() == 1)))<br>
> - return UINT_MAX;<br>
> + // Reconstruct the solution.<br>
> + // FIXME: Add debugging output.<br>
> + Edge *CurrentEdge = &Queue.begin()->second.second;<br>
> + while (CurrentEdge->second != NULL) {<br>
> + LineState CurrentState = *CurrentEdge->second;<br>
> + addTokenToState(CurrentEdge->first, false, CurrentState);<br>
> + CurrentEdge = &Seen[*CurrentEdge->second];<br>
> + }<br>
><br>
> - unsigned CurrentPenalty = 0;<br>
> - if (NewLine)<br>
> - CurrentPenalty += Parameters.PenaltyIndentLevel * State.Stack.size() +<br>
> - State.NextToken->SplitPenalty;<br>
> + // Return the column after the last token of the solution.<br>
> + return Queue.begin()->second.first.Column;<br>
> + }<br>
><br>
> + /// \brief Add the following state to the analysis queue \p Queue.<br>
> + ///<br>
> + /// Assume the current state is \p OldState and has been reached with a<br>
> + /// penalty of \p Penalty. Insert a line break if \p NewLine is \c true.<br>
> + void addNextStateToQueue(const LineState &OldState, unsigned Penalty,<br>
> + bool NewLine,<br>
> + std::multimap<unsigned, QueueItem> &Queue) {<br>
> + if (NewLine && !canBreak(OldState))<br>
> + return;<br>
> + if (!NewLine && mustBreak(OldState))<br>
> + return;<br>
> + LineState State(OldState);<br>
> + if (NewLine)<br>
> + Penalty += Parameters.PenaltyIndentLevel * State.Stack.size() +<br>
> + State.NextToken->SplitPenalty;<br>
> addTokenToState(NewLine, true, State);<br>
> -<br>
> - // Exceeding column limit is bad, assign penalty.<br>
> if (State.Column > getColumnLimit()) {<br>
> unsigned ExcessCharacters = State.Column - getColumnLimit();<br>
> - CurrentPenalty += Parameters.PenaltyExcessCharacter * ExcessCharacters;<br>
> + Penalty += Parameters.PenaltyExcessCharacter * ExcessCharacters;<br>
> }<br>
> + Queue.insert(std::pair<unsigned, QueueItem>(<br>
> + Penalty, QueueItem(State, Edge(NewLine, &OldState))));<br>
> + }<br>
> +<br>
> + /// \brief Returns \c true, if a line break after \p State is allowed.<br>
> + bool canBreak(const LineState &State) {<br>
> + if (!State.NextToken->CanBreakBefore &&<br>
> + !(State.NextToken->is(tok::r_brace) &&<br>
> + State.Stack.back().BreakBeforeClosingBrace))<br>
> + return false;<br>
> + // Trying to insert a parameter on a new line if there are already more than<br>
> + // one parameter on the current line is bin packing.<br>
> + if (State.NextToken->Parent->is(tok::comma) &&<br>
> + State.Stack.back().HasMultiParameterLine &&<br>
> + State.Stack.back().AvoidBinPacking)<br>
> + return false;<br>
> + return true;<br>
> + }<br>
><br>
> - if (StopAt <= CurrentPenalty)<br>
> - return UINT_MAX;<br>
> - StopAt -= CurrentPenalty;<br>
> - StateMap::iterator I = Memory.find(State);<br>
> - if (I != Memory.end()) {<br>
> - // If this state has already been examined, we can safely return the<br>
> - // previous result if we<br>
> - // - have not hit the optimatization (and thus returned UINT_MAX) OR<br>
> - // - are now computing for a smaller or equal StopAt.<br>
> - unsigned SavedResult = I->second.first;<br>
> - unsigned SavedStopAt = I->second.second;<br>
> - if (SavedResult != UINT_MAX)<br>
> - return SavedResult + CurrentPenalty;<br>
> - else if (StopAt <= SavedStopAt)<br>
> - return UINT_MAX;<br>
> - }<br>
> -<br>
> - unsigned NoBreak = calcPenalty(State, false, StopAt);<br>
> - unsigned WithBreak = calcPenalty(State, true, std::min(StopAt, NoBreak));<br>
> - unsigned Result = std::min(NoBreak, WithBreak);<br>
> -<br>
> - // We have to store 'Result' without adding 'CurrentPenalty' as the latter<br>
> - // can depend on 'NewLine'.<br>
> - Memory[State] = std::pair<unsigned, unsigned>(Result, StopAt);<br>
><br>
> - return Result == UINT_MAX ? UINT_MAX : Result + CurrentPenalty;<br>
> + /// \brief Returns \c true, if a line break after \p State is mandatory.<br>
> + bool mustBreak(const LineState &State) {<br>
> + if (State.NextToken->MustBreakBefore)<br>
> + return true;<br>
> + if (State.NextToken->is(tok::r_brace) &&<br>
> + State.Stack.back().BreakBeforeClosingBrace)<br>
> + return true;<br>
> + if (State.NextToken->Parent->is(tok::semi) &&<br>
> + State.LineContainsContinuedForLoopSection)<br>
> + return true;<br>
> + if (State.NextToken->Parent->is(tok::comma) &&<br>
> + State.Stack.back().BreakAfterComma &&<br>
> + State.NextToken->Type != TT_LineComment)<br>
> + return true;<br>
> + if ((State.NextToken->Type == TT_CtorInitializerColon ||<br>
> + (State.NextToken->Parent->ClosesTemplateDeclaration &&<br>
> + State.Stack.size() == 1)))<br>
> + return true;<br>
> + return false;<br>
> }<br>
><br>
> FormatStyle Style;<br>
> @@ -694,10 +703,6 @@ private:<br>
> const AnnotatedToken &RootToken;<br>
> WhitespaceManager &Whitespaces;<br>
><br>
> - // A map from an indent state to a pair (Result, Used-StopAt).<br>
> - typedef std::map<LineState, std::pair<unsigned, unsigned> > StateMap;<br>
> - StateMap Memory;<br>
> -<br>
> OptimizationParameters Parameters;<br>
> };<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>
<br>
<br>
<br>
</div></div>--<br>
Timur Iskhodzhanov,<br>
Google Russia<br>
</blockquote></div><br></div>