[LLVMdev] DenseMap iterator constness fix

[Jeffrey Yasskin]

Whoops! Thanks for the ping! I've committed your patches as r86636 and r86638.

On Mon, Nov 9, 2009 at 1:58 AM, Victor Zverovich
<victor.zverovich at googlemail.com> wrote:
> Reminding about the patches...
> Is there a problem with them or simply nobody have looked at them since?
>
> Victor
>
> 2009/11/4 Victor Zverovich <victor.zverovich at googlemail.com>
>>
>> Good catch! I meant "for iterator" of course. Attached is a corrected
>> patch together with an old patch for clang just to keep them together.
>> Could someone commit these, please?
>> Thanks,
>> Victor
>> 2009/11/4 Jeffrey Yasskin <jyasskin at google.com>
>>>
>>> +  // Otherwise this is a copy constructor for const_iterator.
>>>
>>> Do you mean "for iterator"?
>>>
>>> Otherwise, looks good to me. If you can commit this, please do.
>>> Otherwise, someone else should as I'm not going to be around tomorrow.
>>>
>>> On Wed, Nov 4, 2009 at 12:27 AM, Victor Zverovich
>>> <victor.zverovich at googlemail.com> wrote:
>>> > Hi Jeffrey,
>>> > You are right that the generated copy constructor is used for
>>> > const_iterator. I have added a comment clarifying this. Also I have
>>> > added
>>> > the tests you suggested and corrected the comparison operators. Please
>>> > find
>>> > attached the updated patches.
>>> > Best regards,
>>> > Victor
>>> > 2009/11/3 Jeffrey Yasskin <jyasskin at google.com>
>>> >>
>>> >> +template <bool, typename True, typename False>
>>> >> +struct If { typedef True Result; };
>>> >> +
>>> >> +template <typename True, typename False>
>>> >> +struct If<false, True, False> { typedef False Result; };
>>> >>
>>> >> These should probably go into include/llvm/Support/type_traits.h. In
>>> >> C++0x, this is named "conditional" (section 20.6.7), so I think you
>>> >> should use the same name, despite the standard committee's bad taste.
>>> >>
>>> >> +  DenseMapIterator(const DenseMapIterator<KeyT, ValueT,
>>> >> +                                          KeyInfoT, ValueInfoT,
>>> >> false>&
>>> >> I)
>>> >>
>>> >> This looks like it will make it impossible to copy const_iterators. I
>>> >> guess it doesn't because the copy-constructor is auto-generated, but
>>> >> please comment that and add tests for it and for the non-const->const
>>> >> conversion to unittests/ADT/DenseMapTest.cpp.
>>> >>
>>> >> Otherwise, the patches just change iterator to const_iterator, which
>>> >> looks
>>> >> fine.
>>> >>
>>> >> On Tue, Nov 3, 2009 at 3:09 AM, Victor Zverovich
>>> >> <victor.zverovich at googlemail.com> wrote:
>>> >> > Dear all,
>>> >> > The first of the proposed patches (DenseMapIterator.patch) forbids
>>> >> > implicit
>>> >> > conversion of DenseMap::const_iterator to DenseMap::iterator which
>>> >> > was
>>> >> > possible because DenseMapIterator inherited (publicly) from
>>> >> > DenseMapConstIterator. Conversion the other way around is now
>>> >> > allowed as
>>> >> > one
>>> >> > may expect.
>>> >> > The template DenseMapConstIterator is removed and the template
>>> >> > parameter IsConst which specifies whether the iterator is constant
>>> >> > is added
>>> >> > to DenseMapIterator.
>>> >> > Actually IsConst parameter is not necessary since the constness can
>>> >> > be
>>> >> > determined from KeyT but this is not relevant to the fix and can be
>>> >> > addressed later.
>>> >> > The second patch (DenseMapIterator-clang.patch) corrects clang's
>>> >> > usage
>>> >> > of
>>> >> > DenseMap iterators.
>>> >> > Best regards,
>>> >> > Victor
>>> >> > _______________________________________________
>>> >> > LLVM Developers mailing list
>>> >> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>> >> >
>>> >> >
>>> >
>>> >
>>
>
>