Patch [Fixes #5961]: Disallow comparisons of differing iterator types in ptr_container

[Rob Desbois <rob.desbois@…>]

Patch removes support for comparing differing iterator types from ptr_container's void_ptr_iterator. The comparators in question are == != < <= > >=.

Changes are:

• Add container type template parameter to void_ptr_iterator. This is what ensures compared iterators of differing types are still for the same referenced container type.

• Add self type as template argument to base class for containers ptr_array, ptr_circular_buffer, ptr_deque, ptr_list, ptr_set, ptr_multiset, ptr_unordered_set, ptr_unordered_multiset, and ptr_vector

• Pass container type template parameter from sequence_config and set_config to iterator typedefs, from ptr_sequence_adapter to reversible_ptr_container, and from ptr_set_adapter and ptr_multiset_adapter through ptr_set_adapter_base to associative_ptr_container

[Rob Desbois <rob.desbois@…>]

Patch to ptr_container to fix #5961

[Rob Desbois <rob.desbois@…>]

Patch to ptr_container to fix #5961

[Rob Desbois <rob.desbois@…>]

Apologies - patch file unintentionally duplicated.

[anonymous]

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

[Rob Desbois <rob.desbois@…>]

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]

Hm.

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

-Thorsten

[Rob Desbois <rob.desbois@…>]

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]

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.

[Rob Desbois <rob.desbois@…>]

@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]

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.

[Rob Desbois <rob.desbois@…>]

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

[Rob Desbois <rob.desbois@…>]

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.

[Rob Desbois <rob.desbois@…>]

Revised patch obsoletes earlier patches

[tottosen]

Excellent!

I'll try to apply this over the weekend.

Thanks

-Thorsten