<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 2, 2015 at 9:27 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":3qb" class="a3s" style="overflow:hidden">+sanjoy (somehow I dropped you).<br>
<div><div class="h5"><br>
> On 2015 Mar 2, at 20:38, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
><br>
>> On 2015 Mar 2, at 20:37, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
>><br>
>><br>
>> On Mon, Mar 2, 2015 at 8:35 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>> For `==` and `!=, you shouldn't worry about whether the handle is in sync<br>
>>> with the debug base -- if anything, you should just check that the two<br>
>>> handles are pointing at the same debug base (and have the same epoch as<br>
>>> each other).  (But I still don't see how you've modified `==` or `!=`.)<br>
>>><br>
>>> The comparison operators call operator-><br>
>><br>
>> (Sorry, should have just looked at the code myself.)<br>
>><br>
>> That's silly though.  They should just be:<br>
>><br>
>>    bool operator==(const ConstIterator &RHS) const { return Ptr == RHS.Ptr; }<br>
>>    bool operator!=(const ConstIterator &RHS) const { return Ptr != RHS.Ptr; }<br>
>><br>
>> Calling `->` is needlessly complicated.<br>
>><br>
>> Sure.<br>
>><br>
>> But I think we *should* check the epoch here as comparing an invalid iterator with a valid iterator should also be caught. I would check that the address of the epoch are the same, and if they are non-null, that all three epoch's (the pointed too and both iterator's copies) are in sync.<br>
><br>
> SGTM.<br>
<br>
</div></div>Actually, I wonder if this check is too strict.  Consider an<br>
`erase()` loop:<br>
<span class=""><br>
    for (auto I = M.begin(), E = M.end(); I != E;)<br>
      if (foo(*I))<br>
        I = M.erase(I);<br>
      else<br>
        ++I;<br>
<br>
</span>Although it's not part of this patch, I think we should change<br>
`erase()` to bump the epoch in a follow-up patch (at the very<br>
least, it should bump the epoch on `SmallPtrSet::erase()`).<br>
With your strict semantics, `I` and `E` wouldn't be comparable<br>
after an `erase()` call since `E`'s epoch wouldn't be bumped.<br>
<br>
I'd rather just check that the addresses of the epochs are the<br>
same, and skip the check on the epochs.</div></blockquote></div></div><br><div>But preserving the end iterator isn't valid if you grow the container. I think you're focusing a bit too much on this one pattern here...</div><div><br></div><div>If you have two different classes of iterator (end iterators and all-other-iterators) which are invalidated differently, then you need two epochs. I don't think there is any getting around that really. I think we either need to admit that (and possibly *not* check end iterators for invalidation or make the end iterators really "durable" and never invalidated by always being "null") or we need to not rely on the end iterator being preserved in this code snippet.</div><div><br></div><div>Either way, I think that discussion is best had separately when the erase invalidation is considered.</div></div>