[PATCH] RFC: fail-fast iterators for DenseMap

[Chandler Carruth]

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") or we need to not rely on the end
iterator being preserved in this code snippet.

Either way, I think that discussion is best had separately when the erase
invalidation is considered.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150302/d565ba17/attachment.html>