[PATCH] D21866: [ADT] Add a new data structure for managing a priority worklist where re-insertion of entries into the worklist moves them to the end.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:28
@@ +27,3 @@
+
+/// \brief A FILO worklist that prioritizes on re-insertion without duplication.
+///
----------------
sanjoy wrote:
> Don't we have autobrief these days?
I just forgot. Fixed.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:40
@@ +39,3 @@
+/// The type \c T must be default constructable to a null value that will be
+/// ignored. It is an error to insert such a value, and popping elements will
+/// never produce such a value. It is expected to be used with common nullable
----------------
sanjoy wrote:
> Can we re-use `DenseMapInfo<T>` for this (say the tombstone value)?
We could I guess, but I expect these to essentially always be pointers, and the dense map empty and tombstones for those are going to be less efficient than testing for zero... I mean, probably it doesn't matter, but T() more accurately matches the naive code I would have written by hand and am packaging up here and so that's what I used.

If you or others feel strongly, I would use the empty key as these are in fact empty slots.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:88
@@ +87,3 @@
+  /// \returns true if the element was inserted into the PriorityWorklist.
+  bool insert(const value_type &X) {
+    auto InsertResult = M.insert({X, V.size()});
----------------
sanjoy wrote:
> Why `T` in one place and `value_type` in another?
the value_type came from SetVector. I prefer the easier to read T personally, made it consistent.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:95
@@ +94,3 @@
+    }
+
+    auto &Index = InsertResult.first->second;
----------------
sanjoy wrote:
> We should have a way to fail an assert if someone tries to insert the sentinel "null" value.
Done.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:110-114
@@ +109,7 @@
+    assert(!empty() && "Cannot remove an element when empty!");
+    assert((bool)(back()) && "Cannot have a null element at the back!");
+    M.erase(back());
+    do {
+      V.pop_back();
+    } while (!V.empty() && !V.back());
+  }
----------------
MatzeB wrote:
> Should this rather be V.back() == T()? Or alternatively you could document that T needs to be implicitely convertible to bool with everything but the NULL value being false...
> 
> (Similar in the assert and another instance later).
Yes, good catch. Fixed here and elsewhere.

================
Comment at: include/llvm/ADT/PriorityWorklist.h:177
@@ +176,3 @@
+  ///
+  /// This predicate wraps a predicate suitable for use with std::remove_if to
+  /// call M.erase(x) on each element which is slated for removal.
----------------
sanjoy wrote:
> This is only to avoid templates and lamda, right?  If so, would be nice to state that explicitly.
I guess, this was copied directly from SetVector. I can just delete all of this until someone needs it if you like (my one user doesn't need it).


http://reviews.llvm.org/D21866