[LLVMdev] DenseMap iterator constness fix

[Jeffrey Yasskin]

+  // 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
>> >
>> >
>
>