[PATCH] D18281: [SetVector] Add erase() method
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 11:05:45 PDT 2016
dblaikie added inline comments.
================
Comment at: include/llvm/ADT/SetVector.h:159-165
@@ +158,9 @@
+ iterator erase(iterator I) {
+ const key_type &V = *I;
+ typename vector_type::iterator VI =
+ std::find(vector_.begin(), vector_.end(), V);
+ assert(VI != vector_.end() && "Iterator to erase is out of bounds.");
+ assert(set_.count(V) && "Corrupted SetVector instances!");
+ set_.erase(V);
+ return vector_.erase(VI);
+ }
----------------
This seems like it's doing extra work to find the element in the vector, given an iterator in the vector to begin with - right?
Should just be:
set_.erase(V);
return vector_.erase(I);
or am I missing something? (don't mind if you keep the assert(set_.count(V)))
================
Comment at: unittests/ADT/SetVectorTest.cpp:19
@@ +18,3 @@
+
+void checkEraseAndIteratorsForSetVector() {
+ SetVector<int> S;
----------------
I'd just make these two named functions into test functions - rather than having the test function call them.
Also, seems like these tests are testing a whole bunch of other stuff (which, granted, isn't tested elsewhere - since SetVector had no unit tests - but if it's going to be tested should probably be tested separately (& in a separate commit))
So I'd probably simplify these tests down quite a bit:
TEST(SetVector, EraseTest) {
SetVector<int> 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.front());
EXPECT_EQ(2, S.back());
}
& I wouldn't bother testing SmallSetVector - you only changed SetVector, there's no need to test every possible instantiation of it.
http://reviews.llvm.org/D18281
More information about the llvm-commits
mailing list