[PATCH] RFC: fail-fast iterators for DenseMap
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Mar 2 21:37:52 PST 2015
> On 2015 Mar 2, at 21:36, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
>
> I'd say we should write the erase loop as:
>
> for (auto I = M.begin(), E = M.end(); I != E;)
> if (foo(*I))
> (I,E) = M.erase(I);
> else
> ++I;
This would be surprising. I'd rather match the STL here.
>
> 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.
More information about the llvm-commits
mailing list