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