[llvm-commits] Patch: SparseMultiSet

[Jakob Stoklund Olesen]

On Jan 16, 2013, at 10:39 AM, Michael Ilseman <milseman at apple.com> wrote:

> Attached is a patch introducing a new data structure, the SparseMultiSet, and changes to the MI scheduler to use it.
> 
> A SparseMultiSet adds multiset behavior to SparseSet, while retaining SparseSet's desirable properties. Essentially, SparseMultiSet provides multiset behavior by storing its dense data in doubly linked lists that are inlined into the dense vector. This allows it to provide good data locality as well as vector-like constant-time clear() and fast constant time find(), insert(), and erase(). It also allows SparseMultiSet to have a builtin recycler rather than keeping SparseSet's behavior of always swapping upon removal, which allows it to preserve more iterators. It's often a better alternative to a SparseSet of a growable container or vector-of-vector.

+    return (Dense.begin() + N.Prev) == &N;

AFAICT, this assumes that begin() is a pointer, but it could be some iterator class.

+    const iterator_base &operator=(const iterator_base &RHS) {
+      Dense = RHS.Dense;

Dense is a reference, so this assigns the whole array. Please make sure you have a test case that catches this.

+    /// Increment and decrement operators
+    iterator_base &operator--() { // predecrement - Back up
+      assert(isValid() && "Decrementing an invalid iterator");
+
+      // If we're at the end, then interpret our index as a previous index
+      if (IsEnd)
+        IsEnd = false;

How do you know Idx is still valid here? Please add a test that decrements end() after erasing elements.

+  std::pair<iterator,iterator> equal_range(const KeyT &K) {
+    iterator I = find(K);
+    if (I == end())
+      return make_pair(I, I);
+
+    iterator B = I--;
+    return make_pair(B, ++I);
+  }

This looks very odd. I don't think the iterator class should expose to clients that it is a circular linked list. It would be better is --I on find() were an assertion. Use private methods to access the linked list.

It looks like you need better test coverage for all the corner cases.

/jakob