[LLVMdev] DenseMap iterator constness fix

[Victor Zverovich]

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
>> >> >
>> >> >
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091109/1577067e/attachment.html>