[PATCH] RFC: fail-fast iterators for DenseMap
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Mar 2 20:35:24 PST 2015
> On 2015 Mar 2, at 20:32, Chandler Carruth <chandlerc at google.com> wrote:
>
>
> On Mon, Mar 2, 2015 at 8:20 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > On 2015 Mar 2, at 20:12, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> >
> >> Comparing two default constructed iterators should be valid, but
> >> I don't understand how your patch affects that operation. Am I
> >> missing something obvious?
> >
> > In the current implementation, default constructed iterators have a
> > nullptr epoch address, so looking up their epoch segfaults.
>
> What I don't see is where you change the comparison of `DenseMapIterator`
> to depend on the epoch. AFAICT, you only modified `++`, `*`, and `->`,
> none of which should be called from `==` or `!=`.
>
> >
> > In any case, the fix should be easy; I just wanted a clarification on
> > what the intended semantics are.
> >
>
> 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.
More information about the llvm-commits
mailing list