[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.

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.
sanjoy added a comment.

Drop by comments.


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

================
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
----------------
Can we re-use `DenseMapInfo<T>` for this (say the tombstone value)?

================
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()});
----------------
Why `T` in one place and `value_type` in another?

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

================
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.
----------------
This is only to avoid templates and lamda, right?  If so, would be nice to state that explicitly.


http://reviews.llvm.org/D21866