Reminding about the patches...<div>Is there a problem with them or simply nobody have looked at them since?<br><div><br></div><div>Victor<br><div><div><br><div class="gmail_quote">2009/11/4 Victor Zverovich <span dir="ltr"><<a href="mailto:victor.zverovich@googlemail.com">victor.zverovich@googlemail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">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.<div>
Could someone commit these, please?</div><div><br></div><div>Thanks,</div>
<div>Victor</div><br><div class="gmail_quote">2009/11/4 Jeffrey Yasskin <span dir="ltr"><<a href="mailto:jyasskin@google.com" target="_blank">jyasskin@google.com</a>></span><div><div></div><div class="h5"><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

+  // Otherwise this is a copy constructor for const_iterator.<br>
<br>
Do you mean "for iterator"?<br>
<br>
Otherwise, looks good to me. If you can commit this, please do.<br>
Otherwise, someone else should as I'm not going to be around tomorrow.<br>
<br>
On Wed, Nov 4, 2009 at 12:27 AM, Victor Zverovich<br>
<div><div></div><div><<a href="mailto:victor.zverovich@googlemail.com" target="_blank">victor.zverovich@googlemail.com</a>> wrote:<br>
> Hi Jeffrey,<br>
> You are right that the generated copy constructor is used for<br>
> const_iterator. I have added a comment clarifying this. Also I have added<br>
> the tests you suggested and corrected the comparison operators. Please find<br>
> attached the updated patches.<br>
> Best regards,<br>
> Victor<br>
> 2009/11/3 Jeffrey Yasskin <<a href="mailto:jyasskin@google.com" target="_blank">jyasskin@google.com</a>><br>
>><br>
>> +template <bool, typename True, typename False><br>
>> +struct If { typedef True Result; };<br>
>> +<br>
>> +template <typename True, typename False><br>
>> +struct If<false, True, False> { typedef False Result; };<br>
>><br>
>> These should probably go into include/llvm/Support/type_traits.h. In<br>
>> C++0x, this is named "conditional" (section 20.6.7), so I think you<br>
>> should use the same name, despite the standard committee's bad taste.<br>
>><br>
>> +  DenseMapIterator(const DenseMapIterator<KeyT, ValueT,<br>
>> +                                          KeyInfoT, ValueInfoT, false>&<br>
>> I)<br>
>><br>
>> This looks like it will make it impossible to copy const_iterators. I<br>
>> guess it doesn't because the copy-constructor is auto-generated, but<br>
>> please comment that and add tests for it and for the non-const->const<br>
>> conversion to unittests/ADT/DenseMapTest.cpp.<br>
>><br>
>> Otherwise, the patches just change iterator to const_iterator, which looks<br>
>> fine.<br>
>><br>
>> On Tue, Nov 3, 2009 at 3:09 AM, Victor Zverovich<br>
>> <<a href="mailto:victor.zverovich@googlemail.com" target="_blank">victor.zverovich@googlemail.com</a>> wrote:<br>
>> > Dear all,<br>
>> > The first of the proposed patches (DenseMapIterator.patch) forbids<br>
>> > implicit<br>
>> > conversion of DenseMap::const_iterator to DenseMap::iterator which was<br>
>> > possible because DenseMapIterator inherited (publicly) from<br>
>> > DenseMapConstIterator. Conversion the other way around is now allowed as<br>
>> > one<br>
>> > may expect.<br>
>> > The template DenseMapConstIterator is removed and the template<br>
>> > parameter IsConst which specifies whether the iterator is constant<br>
>> > is added<br>
>> > to DenseMapIterator.<br>
>> > Actually IsConst parameter is not necessary since the constness can be<br>
>> > determined from KeyT but this is not relevant to the fix and can be<br>
>> > addressed later.<br>
>> > The second patch (DenseMapIterator-clang.patch) corrects clang's usage<br>
>> > of<br>
>> > DenseMap iterators.<br>
>> > Best regards,<br>
>> > Victor<br>
>> > _______________________________________________<br>
>> > LLVM Developers mailing list<br>
>> > <a href="mailto:LLVMdev@cs.uiuc.edu" target="_blank">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div></div></div><br>
</blockquote></div><br></div></div></div></div>