djasper added inline comments.

================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:723
       FormatDecision LastFormat = Node->State.NextToken->Decision;
       if (LastFormat == FD_Unformatted || LastFormat == FD_Continue)
+        addNextStateToQueue(Penalty, Node, /*NewLine=*/false,
----------------
This is almost certainly a no-go. It turns the algorithm from exploring a state 
space with a branching factor of 2 into something with a branching factor of 4.

Even assuming we can perfectly reuse results from computations (or in other 
words hit the same state through different path using Dijkstra's algorithm), 
the additional bool in StateNode doubles the number of states we are going to 
visit. You don't even seem to make a distinction of whether the token under 
investigation can possibly be split. You do look at NoLineBreak(InOperand), but 
even if those are false, the vast majority of tokens will never be split.

However, even if you narrow that down, I am not convinced that fixing this 
inconsistency around the formatting penalties is worth the runtime performance 
hit. I am generally fine with changing the behavior in the way you are 
proposing, but only if it has zero (negative) effect on performance.


================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:764
       return;
+    if (!BreakToken && !Indenter->canSplit(PreviousNode->State))
+      return;
----------------
It's not clear to me why you'd return here.


https://reviews.llvm.org/D33589



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to