[LLVMdev] DenseMap iterator constness fix

[Victor Zverovich]

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/20091104/f08e98c7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DenseMapIterator2.patch
Type: text/x-patch
Size: 15541 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091104/f08e98c7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DenseMapIterator-clang2.patch
Type: text/x-patch
Size: 3293 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091104/f08e98c7/attachment-0001.bin>