[PATCH] RFC: fail-fast iterators for DenseMap

[Duncan P. N. Exon Smith]

> On 2015 Mar 2, at 21:36, Chandler Carruth <chandlerc at google.com> wrote:
> 
> 
> On Mon, Mar 2, 2015 at 9:27 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> +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.
> 
> 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...
> 
> 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")

Either of these options would be fine with me.

> or we need to not rely on the end iterator being preserved in this code snippet.

I think it's unnecessarily complicated to deviate from the STL
API here.

> 
> Either way, I think that discussion is best had separately when the erase invalidation is considered.

Sure.