[PATCH] An attempt to make the search algorithm easier to understand.

Daniel Jasper djasper at google.com
Tue Feb 12 23:26:24 PST 2013


  Very nice! I think a few names could be more intuitive, but other than that looks awesome!


================
Comment at: lib/Format/Format.cpp:673
@@ +672,3 @@
+    // Increasing count of \c StateEdge items we have created. This is used
+    // to stabilize the priority queue.
+    unsigned Count = 0;
----------------
How about: "This is used to create a deterministic order independent of the container."

================
Comment at: lib/Format/Format.cpp:654
@@ +653,3 @@
+  /// that break the line as late as possible.
+  typedef std::pair<unsigned, unsigned> StablePenalty;
+
----------------
Maybe OrderedPenalty?

================
Comment at: lib/Format/Format.cpp:656
@@ +655,3 @@
+
+  /// \brief An item in the prioritized BFS search queue. The \c StateEdge 
+  typedef std::pair<StablePenalty, StateEdge *> QueueItem;
----------------
Incomplete comment.

================
Comment at: lib/Format/Format.cpp:718
@@ +717,3 @@
+
+  void apply(LineState &State, StateEdge *Current) {
+    if (Current->Previous == NULL)
----------------
apply seems a very generic name. How about useEdge?

================
Comment at: lib/Format/Format.cpp:738
@@ +737,3 @@
+                           QueueType &Queue, unsigned &Count, unsigned Penalty,
+                           StateEdge *Old, bool NewLine) {
+    if (NewLine && !canBreak(Old->State))
----------------
"Old" does not mean much to me. Maybe PreviousEdge.

================
Comment at: lib/Format/Format.cpp:644
@@ +643,3 @@
+    }
+    LineState State;
+    bool NewLine;
----------------
Might be nice to reflect in the name that this is the node that this edge leads to. Maybe "NextState"?


http://llvm-reviews.chandlerc.com/D398

BRANCH
  format-penalty

ARCANIST PROJECT
  clang



More information about the cfe-commits mailing list