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
DenseMapIterator.patch (13.3 KB)
DenseMapIterator-clang.patch (3.25 KB)
+template <bool, typename True, typename False>
+struct If { typedef True Result; };
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@google.com>
DenseMapIterator2.patch (15.2 KB)
DenseMapIterator-clang2.patch (3.22 KB)
+ // 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.
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@google.com>
DenseMapIterator2.patch (15.2 KB)
DenseMapIterator-clang2.patch (3.22 KB)
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@googlemail.com>
Whoops! Thanks for the ping! I've committed your patches as r86636 and r86638.