[PATCH] D18281: [SetVector] Add erase() method

[David Blaikie via llvm-commits]

dblaikie accepted this revision.
dblaikie added a reviewer: dblaikie.
dblaikie added a comment.

Looks good - please commit after removing the extra test & asserts.


================
Comment at: include/llvm/ADT/SetVector.h:159-160
@@ +158,4 @@
+  iterator erase(iterator I) {
+    assert(I >= this->begin() && "Iterator to erase is out of bounds.");
+    assert(I < this->end() && "Erasing at past-the-end iterator.");
+    const key_type &V = *I;
----------------
While I see these comparisons in SmallVector (which perhaps is where you got the idea) they're actually unspecified (see C++ standard 5.9 [expr.rel]/2) - if we used std::less they would be well defined, but I think we could just leave them out.

================
Comment at: unittests/ADT/SetVectorTest.cpp:34-49
@@ +33,17 @@
+}
+
+TEST(SmallSetVector, EraseTest) {
+  SmallSetVector<int, 2> S;
+  S.insert(0);
+  S.insert(1);
+  S.insert(2);
+
+  auto I = S.erase(std::next(S.begin()));
+
+  // Test that the returned iterator is the expected one-after-erase
+  // and the size/contents is the expected sequence {0, 2}.
+  EXPECT_EQ(std::next(S.begin()), I);
+  EXPECT_EQ(2u, S.size());
+  EXPECT_EQ(0, *S.begin());
+  EXPECT_EQ(2, *std::next(S.begin()));
+}
----------------
Remove this test - I don't think we gain enough benefit/improved coverage to keep it.


http://reviews.llvm.org/D18281