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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091103/51699ffd/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: DenseMapIterator.patch Type: application/octet-stream Size: 13668 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091103/51699ffd/attachment.obj> -------------- next part -------------- A non-text attachment was scrubbed... Name: DenseMapIterator-clang.patch Type: application/octet-stream Size: 3323 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20091103/51699ffd/attachment-0001.obj>
+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 > >
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>