[PATCH] D18281: [SetVector] Add erase() method
Jun Bum Lim via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 24 11:54:41 PDT 2016
junbuml 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);
+ }
----------------
dblaikie wrote:
> 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)))
We cannot use the given iterator I directly because it's const_iterator which cannot be used in vector_.erase(). Please let me know if there is any better way to get a non const iterator which we can pass to vector_.erase().
http://reviews.llvm.org/D18281
More information about the llvm-commits
mailing list