<div dir="ltr">SGTM<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 24, 2016 at 12:40 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Thu, Mar 24, 2016 at 12:03 PM, Daniel Berlin via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Thu, Mar 24, 2016 at 11:54 AM, Jun Bum Lim via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">junbuml added inline comments.<br>
<span><br>
================<br>
Comment at: include/llvm/ADT/SetVector.h:159-165<br>
@@ +158,9 @@<br>
</span><span>+ iterator erase(iterator I) {<br>
+ const key_type &V = *I;<br>
+ typename vector_type::iterator VI =<br>
+ std::find(vector_.begin(), vector_.end(), V);<br>
+ assert(VI != vector_.end() && "Iterator to erase is out of bounds.");<br>
</span><span>+ assert(set_.count(V) && "Corrupted SetVector instances!");<br>
+ set_.erase(V);<br>
+ return vector_.erase(VI);<br>
+ }<br>
</span>----------------<br>
<span>dblaikie wrote:<br>
> 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?<br>
><br>
> Should just be:<br>
><br>
> set_.erase(V);<br>
> return vector_.erase(I);<br>
><br>
> or am I missing something? (don't mind if you keep the assert(set_.count(V)))<br>
</span>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().<br></blockquote><div><br></div></span><div>Errr, it should work for C++11, as the erase member will take a const_iterator.</div><div>If not, the range version should *definitely work* IE vector_.erase(I, I).</div></div></div></div></blockquote><div><br></div></div></div><div>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.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
<br>
<a href="http://reviews.llvm.org/D18281" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18281</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></span></div><br></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></span></div><br></div></div>
</blockquote></div><br></div>