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

[Daniel Berlin via llvm-commits]

SGTM


On Thu, Mar 24, 2016 at 12:40 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Mar 24, 2016 at 12:03 PM, Daniel Berlin via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>>
>> On Thu, Mar 24, 2016 at 11:54 AM, Jun Bum Lim via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> 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().
>>>
>>
>> Errr, it should work for C++11, as the erase member will take a
>> const_iterator.
>> If not, the range version should *definitely work* IE vector_.erase(I, I).
>>
>
> Yep, had the same thought - but apparently in C++98 both range and
> singular erase took non-const iterators and we mirrored that in
> SmallVector. I'm fixing SmalLVector to match the C++11 idea (const_iterator
> for both cases) now.
>
>
>>
>>
>>>
>>>
>>> http://reviews.llvm.org/D18281
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160324/a011808a/attachment.html>