Boost C++ Libraries: Ticket #5963: Patch [Fixes #5961]: Disallow comparisons of differing iterator types in ptr_container

attachment set

Thu, 29 Sep 2011 14:11:13 GMT

attachment → ptr_container.vp_iterator.comparators.patch

Patch to ptr_container to fix #5961

attachment set

Thu, 29 Sep 2011 14:11:17 GMT

attachment → ptr_container.vp_iterator.comparators.2.patch

Patch to ptr_container to fix #5961

Thu, 29 Sep 2011 14:13:00 GMT

Apologies - patch file unintentionally duplicated.

anonymous — Thu, 29 Sep 2011 14:21:16 GMT

Hi,

Thanks for the patch. However, I do not think its the right way to do it. I think it generates too many iterator types, and I think there must be a much simpler fix.

I'm thinking out of the box, partially because I cannot remember why the code takes parameters U and T. Maybe it is because that U and T are always either X or const X.

That is, it may be possible to fix the problem by using enable_if<> that requires that is_same< remove_const<U>, remove_const<T> >::value is true.

-Thorsten

Thu, 29 Sep 2011 16:14:11 GMT

Ah yes, didn't think of the number of resulting types that would be generated..

Using enable_if<> etc. as you suggest looks good. Since we can't use enable_if<> either as a dummy parameter (since these are binary operators) or as the return value (since we're using it) I'd suggest we create a helper traits that defines non-operator equivalents of these operations. Would you agree that's the way to go?

(NB: we technically /could/ put enable_if<> in the return, but that would make for a horrible public interface.)

anonymous — Thu, 29 Sep 2011 16:21:03 GMT

Hm.

Maybe it would be sufficient to put a static assert + comment in the body of the involved functions.

-Thorsten

Thu, 29 Sep 2011 16:56:16 GMT

Yes that would be simpler..

How about the following?

   // Helper struct that determines compatibility
   template<typename T, typename U>
   struct_is_compatible
   {
      static const bool value = typename boost::is_same< typename boost::remove_const<T>::type, typename boost::remove_const<U>::type >::value;
   };
   // And add the assertion to each operator like this:
   template< class VoidIterT, class T, class VoidIterU, class U >
   inline bool operator==( const void_ptr_iterator<VoidIterT,T>& l,
                           const void_ptr_iterator<VoidIterU,U>& r )
   {
      BOOST_STATIC_ASSERT((is_compatible<T, U>::value));
      // ...
   }

where should the helper struct go? I'm guessing directly in the boost namespace isn't ideal..

anonymous — Thu, 29 Sep 2011 21:11:47 GMT

Thorsten's enable_if solution has the advantage that instantiation errors will be reported in user's code, whereas the static assert will generate errors in Boost headers, which is not ideal.

Fri, 30 Sep 2011 08:48:10 GMT

@anonymous author of comment 6 - indeed, I agree it's not great having the error appear in the header, but due to reasons outlined in comment 3, it's not possible to push this error back to the user code (as far as I know - suggestions welcomed.)

anonymous — Fri, 30 Sep 2011 09:06:24 GMT

I guess the usual way to handle errors in Boost code is to make a clear comment saying why you got this error. E.g. Boost.Serialization and Boost.Range does this.

After all, the important this is that you get an error.

Fri, 30 Sep 2011 09:14:45 GMT

Indeed - since getting the error is after all the point of this, I'll make it use BOOST_STATIC_ASSERT_MSG() to be helpful.

Fri, 30 Sep 2011 10:55:33 GMT

Actually I was talking rubbish earlier, we /can/ use enable_if in the operator because it can wrap any type you pass as the second template param. I've wrapped all the return types in question with an enable_if using the new struct ptr_container_detail::is_compatible.

My own tests and those in /libs/ptr_container/test all check out fine, error messages are helpful and appear for the offending line of user code.

attachment set

Fri, 30 Sep 2011 10:56:13 GMT

attachment → ptr_container.vp_iterator.comparators.3.patch

Revised patch obsoletes earlier patches

tottosen — Fri, 30 Sep 2011 11:01:42 GMT

Excellent!

I'll try to apply this over the weekend.

Thanks

-Thorsten