[PATCH] RFC: fail-fast iterators for DenseMap

[Duncan P. N. Exon Smith]

+sanjoy (somehow I dropped you).

> On 2015 Mar 2, at 20:38, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015 Mar 2, at 20:37, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>> 
>> On Mon, Mar 2, 2015 at 8:35 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> For `==` and `!=, you shouldn't worry about whether the handle is in sync
>>> with the debug base -- if anything, you should just check that the two
>>> handles are pointing at the same debug base (and have the same epoch as
>>> each other).  (But I still don't see how you've modified `==` or `!=`.)
>>> 
>>> The comparison operators call operator->
>> 
>> (Sorry, should have just looked at the code myself.)
>> 
>> That's silly though.  They should just be:
>> 
>>    bool operator==(const ConstIterator &RHS) const { return Ptr == RHS.Ptr; }
>>    bool operator!=(const ConstIterator &RHS) const { return Ptr != RHS.Ptr; }
>> 
>> Calling `->` is needlessly complicated.
>> 
>> Sure.
>> 
>> 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.
> 
> SGTM.

Actually, I wonder if this check is too strict.  Consider an
`erase()` loop:

    for (auto I = M.begin(), E = M.end(); I != E;)
      if (foo(*I))
        I = M.erase(I);
      else
        ++I;

Although it's not part of this patch, I think we should change
`erase()` to bump the epoch in a follow-up patch (at the very
least, it should bump the epoch on `SmallPtrSet::erase()`).
With your strict semantics, `I` and `E` wouldn't be comparable
after an `erase()` call since `E`'s epoch wouldn't be bumped.

I'd rather just check that the addresses of the epochs are the
same, and skip the check on the epochs.