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