*** empty log message ***

[SVN r21519]

doc/interoperability-revisited.rst

@@ -0,0 +1,232 @@
+++++++++++++++++++++++++++++
+ Interoperability Revisited 
+++++++++++++++++++++++++++++
+
+:date: $Date$
+:copyright: Copyright Thomas Witt 2004.
+
+
+Problem
+=======
+
+The current iterator_facade specification makes it unneccessarily tedious to
+implement interoperable iterators.
+
+In the following text a simplified example of the current iterator_facade specification is used to
+illustrate the problem.
+
+In the current specification binary operators are implemented in the following way:
+
+template <class Derived>
+struct Facade
+{
+};
+
+template <class T1, T2>
+struct is_interoperable :
+  or_< 
+       is_convertible<T1, T2>
+     , is_convertible<T2, T1>
+  > 
+{};
+
+template<
+    class Derived1
+  , class Derived2
+>
+enable_if<is_interoperable<Derived1, Derived2>, bool> operator==(
+    Derived1 const& lhs
+  , Derived2 const& rhs
+)
+{
+  return static_cast<Derived1 const&>(lhs).equal_to(static_cast<Derived2 const&(rhs));
+} 
+
+The problem with this is that operator== always forwards to Derived1::equal_to. The net effect is that the
+following "obvious" implementation of to interoperable types does not quite work.
+
+struct Mutable : Facade<Mutable>
+{
+  bool equal_to(Mutable const&);  
+};
+
+struct Constant : Facade<Constant>
+{
+  Constant();
+  Constant(Constant const&);
+  Constant(Mutable const&);
+ 
+  ...
+
+  bool equal_to(Constant const&);  
+};
+
+Constant c;
+Mutable  m;
+
+c == m; // ok, dispatched to Constant::equal_to
+m == c; // !! error, dispatched to Mutable::equal_to
+
+Instead the following "slightly" more complicated implementation is neccessary
+
+struct Mutable : Facade<Mutable>
+{
+  template <class T>
+  enable_if<is_convertible<Mutable, T> || is_convertible<T, Mutable>, bool>::type equal_to(T const&);  
+};
+
+struct Constant : Tag<Constant>
+{
+  Constant();
+  Constant(Constant const&);
+  Constant(Mutable const&);
+
+  template <class T>
+  enable_if<is_convertible<Constant, T> || is_convertible<T, Constant>, bool>::type equal_to(T const&);  
+};
+
+Beside the fact that the code is significantly more complex to understand and to teach there is
+a major design problem lurking here. Note that in both types equal_to is a function template with 
+an unconstrained argument T. This is neccessary so that further types can be made interoperable with
+Mutable or Constant. Would Mutable be defined as  
+
+struct Mutable : Facade<Mutable>
+{
+  bool equal_to(Mutable const&);  
+  bool equal_to(Constant const&);  
+};
+
+Constant and Mutable would still be interoperable but no further interoperable could be added 
+without changing Mutable. Even if this would be considered acceptable the current specification forces
+a two way dependency between interoperable types. Note in the templated equal_to case this dependency 
+is implicitly created when specializing equal_to.
+
+Solution
+========
+
+The two way dependency can be avoided by enabling type conversion in the binary operator
+implementation. Note that this is the usual way interoperability betwween types is achieved
+for binary operators and one reason why binary operators are usually implemented as non-members.
+
+A simple implementation of this strategy would look like this
+
+template<
+    class T1
+  , class T2
+>
+struct interoperable_base :
+    if_< 
+        is_convertible<
+            T2
+          , T1
+        >
+      , T1
+      , T2>
+{};
+
+
+template<
+    class Derived1
+  , class Derived2
+>
+enable_if<is_interoperable<Derived1, Derived2>, bool> operator==(
+    Derived1 const& lhs
+  , Derived2 const& rhs
+)
+{
+  typedef interoperable_base<
+              Derived1
+            , Derived2
+          >::type Base;
+
+  return static_cast<Base const&>(lhs).equal_to(static_cast<Derived2 const&(rhs));
+} 
+
+This way our original simple and "obvious" implementation would work again.
+
+c == m; // ok, dispatched to Constant::equal_to
+m == c; // ok, dispatched to Constant::equal_to, m converted to Constant
+
+The backdraw of this approach is that a possibly costly conversion of iterator objects
+is forced on the user even in cases where direct comparison could be implemented
+in a much more efficient way. This problem arises especially for iterator_adaptor
+specializations and can be significantly slow down the iteration over ranges. Given the fact
+that iteration is a very basic operation this possible performance degradation is not 
+acceptable.
+
+Luckily whe can have our cake and eat it by a slightly more clever implementation of the binary 
+operators.
+
+template<
+    class Derived1
+  , class Derived2
+>
+enable_if<is_convertible<Derived2, Derived1>, bool> operator==(
+    Derived1 const& lhs
+  , Derived2 const& rhs
+)
+{
+  return static_cast<Derived1 const&>(lhs).equal_to(static_cast<Derived2 const&(rhs));
+} 
+
+template<
+    class Derived1
+  , class Derived2
+>
+enable_if<is_convertible<Derived1, Derived2>, bool> operator==(
+    Derived1 const& lhs
+  , Derived2 const& rhs
+)
+{
+  return static_cast<Derived2 const&>(rhs).equal_to(static_cast<Derived1 const&(lhs));
+} 
+
+Given our simple and obvious definition of Mutable and Constant nothing has changed yet. 
+
+c == m; // ok, dispatched to Constant::equal_to, m converted to Constant
+m == c; // ok, dispatched to Constant::equal_to, m converted to Constant
+
+But now the user can avoid the type conversion by supplying the appropriate overload in Constant
+
+struct Constant : Facade<Constant>
+{
+  Constant();
+  Constant(Constant const&);
+  Constant(Mutable const&);
+ 
+  ...
+
+  bool equal_to(Constant const&);  
+  bool equal_to(Mutable const&);  
+};
+
+c == m; // ok, dispatched to Constant::equal_to(Mutable const&), no conversion
+m == c; // ok, dispatched to Constant::equal_to(Mutable const&), no conversion
+
+This definition of operator== introduces a possible ambiguity when both types are convertible
+to each other. I don't think this is a problem as this behaviour is the same with concrete types.
+I.e. 
+
+struct A {};
+
+bool operator==(A, A);
+
+struct B { B(A); }; 
+
+bool operator==(B, B);
+
+A a;
+B b(a);
+
+a == b; // error, ambiguous overload
+
+Effect
+======
+
+Iterator implementations using iterator_facade look exactly as if they were
+"hand-implemented" (I am working on better wording).
+
+a) Less burden for the user
+
+b) The definition (standardese) of specialized adpters might be easier 
+   (This has to be proved yet)

doc/iter-issue-list.rst

@@ -83,6 +83,10 @@ N1541 48
 
   **Needs work** (Dave)  I'm not happy with Pete's proposal.
 
+  (thw) Pete is correct with regard to the requirement. Removing the
+  interoperable stuff would be an error. By all means we don't want
+  undefined behaviour here.
+
 9.4 enable_if_convertible unspecified, conflicts with requires 
 ==============================================================
 
@@ -108,6 +112,7 @@ There are two problems. First, enable_if_convertible is never specified, so we d
 know what this is supposed to do. Second: we could reasonably say that this overload should be 
 disabled in certain cases or we could reasonably say that behavior is undefined, but we can<61>t say 
 both. 
+
 Thomas Witt writes that the goal of putting in enable_if_convertible here is to make 
 sure that a specific overload doesn<73>t interfere with the generic case except when that overload 
 makes sense. He agrees that what we currently have is deficient. 
@@ -281,12 +286,10 @@ encouraged to ignore this argument if it won't work right, why is it there?
 :Status: New 
 
 Shortly after N1550 was accepted, we discovered that an iterator's lvalueness can be determined 
-knowing only itsvalue_type. This predicate can be calculated even for old-style iterators (on 
+knowing only its value_type. This predicate can be calculated even for old-style iterators (on 
-N1541 51 
-
 whose reference type the standard places few requirements). A trait in the Boost iterator library 
 does it by relying on the compiler's unwillingness to bind an rvalue to a T& function template 
-parameter. Similarly, it is possible to detect an iterator's readability knowing only itsvalue_type. 
+parameter. Similarly, it is possible to detect an iterator's readability knowing only its value_type. 
 Thus, any interface which asks the user to explicitly describe an iterator's lvalue-ness or 
 readability seems to introduce needless complexity. 
 
@@ -413,7 +416,6 @@ having a hard time justifying their impact on the rest of the proposal(s).
 
 :Proposed Resolution: See the resolution to 9.15.
 
-
 9.19 Non-Uniformity of the "lvalue_iterator Bit" 
 ================================================
 
@@ -472,6 +474,10 @@ be worth giving the names of these tags (and the associated concepts) some extra
     random_access_traversal_tag
 
 
+   ** Needs work ** (thw) I still believe that implicit_traversal_tag is more
+   appropriate than incrementable_traversal_tag
+
+
 9.21 iterator_facade Derived template argument underspecified 
 =============================================================
 
@@ -499,6 +505,12 @@ from a specialization of iterator_facade whose first template argument is Iter."
 awkward, but at the moment I don't see a better way of phrasing it. 
 
 :Proposed resolution:   **Needs work** (Dave) Reword.
+   01/01/04 thw
+   The wording is certainly insufficient. AFAICS there are two issues.
+   First the issue addressed by Pete i.e. specifying the requirements for
+   implementing a valid iterator. The other issue I can see is that we 
+   need to be able to unambigously cast the iterator_facade specialisation
+   to Iter.
 
 9.22 return type of Iterator difference for iterator facade 
 ===========================================================
@@ -529,6 +541,11 @@ right?
 
     typename enable_if_interoperable<Dr1, Dr2, D1>::type
 
+   01/01/04 thw 
+   Almost, the return type should be the difference_type of the
+   converted to iterator. BTW how does std::distance handle
+   different but interoperable iterator types?
+
 
 9.23 Iterator_facade: minor wording Issue 
 =========================================

include/boost/iterator/iterator_facade.hpp

@@ -63,6 +63,72 @@ namespace boost
 #endif
     };
 
+    //
+    // Determines the iterator type that can be
+    // safely used to implement binary operators
+    //
+    template <
+        class Facade1,
+       class Facade2
+    >
+    struct interoperable_base 
+      : mpl::if_<
+            is_convertible<Facade1, Facade2>
+          , Facade2
+          , Facade1
+        >
+    {
+    };
+
+    template <class Facade1, class Facade2>
+    struct difference_type
+    {
+      typedef typename interoperable_base<Facade1, Facade2>::type::difference_type type;
+    };
+
+    template <class Facade1, class Facade2>
+    bool equal(Facade1 const&, Facade2 const&);
+
+    template <class Facade1, class Facade2>
+    typename Facade1::difference_type distance_to(Facade1 const&, Facade2 const&);
+
+    template <bool UseLhsMember>
+    struct facade_binary_operator
+    {
+      template <class Facade1, class Facade2>
+      static bool equal(Facade1 const& lhs,
+                        Facade2 const& rhs)
+      {
+        return ::boost::detail::equal(lhs, rhs);
+      }
+
+      template <class Facade1, class Facade2>
+      static typename Facade1::difference_type distance_to(Facade1 const& lhs,
+                                                         Facade2 const& rhs)
+      {
+        return ::boost::detail::distance_to(lhs, rhs);
+      }
+    };
+    
+    template <>
+    struct facade_binary_operator<false>
+    {
+      template <class Facade1, class Facade2>
+      static bool equal(Facade1 const& lhs,
+                        Facade2 const& rhs)
+      {
+        return ::boost::detail::equal(rhs, lhs);
+      }
+
+      template <class Facade1, class Facade2>
+      static typename Facade2::difference_type distance_to(Facade1 const& lhs,
+                                                         Facade2 const& rhs)
+      {
+        return -::boost::detail::distance_to(rhs, lhs);
+      }
+    };
+    
+
     //
     // Generates associated types for an iterator_facade with the
     // given parameters.
@@ -256,9 +322,11 @@ namespace boost
       BOOST_ITERATOR_FACADE_RELATION(>=)
 #  undef BOOST_ITERATOR_FACADE_RELATION
 
-      BOOST_ITERATOR_FACADE_INTEROP_HEAD(
+      template < class Derived1, class V1, class TC1, class R1, class D1 , class Derived2, class V2, class TC2, class R2, class D2 > friend typename detail::enable_if_interoperable< Derived1, Derived2, typename std::iterator_traits<typename detail::interoperable_base<Derived1, Derived2> ::type > ::difference_type > ::type operator -( iterator_facade<Derived1, V1, TC1, R1, D1> const& lhs , iterator_facade<Derived2, V2, TC2, R2, D2> const& rhs);
-          friend, -, typename Derived1::difference_type)
+
-      ;
+//       BOOST_ITERATOR_FACADE_INTEROP_HEAD(
+//                                          friend, -, (typename std::iterator_traits<typename detail::interoperable_base<Derived1, Derived2>::type >::difference_type))
+//       ;
 
       BOOST_ITERATOR_FACADE_PLUS_HEAD(
           friend                                
@@ -297,7 +365,7 @@ namespace boost
       template <class Facade1, class Facade2>
       static bool equal(Facade1 const& f1, Facade2 const& f2)
       {
-          return f1.equal(f2);
+        return detail::facade_binary_operator< is_convertible< Facade2, Facade1 >::value >::equal(f1, f2);
       }
 
       template <class Facade>
@@ -307,17 +375,51 @@ namespace boost
       }
 
       template <class Facade1, class Facade2>
-      static typename Facade1::difference_type distance_to(
+      static typename detail::difference_type<Facade1, Facade2>::type distance_to(
                                                            Facade1 const& f1, Facade2 const& f2)
       {
-          return f1.distance_to(f2);
+        return detail::facade_binary_operator< is_convertible< Facade2, Facade1 >::value >::distance_to(f1, f2);
       }
 
    private:
       // objects of this class are useless
       iterator_core_access(); //undefined
+
+    // We would need template friends for these to be private
+   public:
+      template <class Facade1, class Facade2>
+      static bool equal_fwd(Facade1 const& f1, Facade2 const& f2)
+      {
+          return f1.equal(f2);
+      }
+
+      template <class Facade1, class Facade2>
+      static typename Facade1::difference_type distance_to_fwd(
+                                                           Facade1 const& f1, Facade2 const& f2)
+      {
+          return f1.distance_to(f2);
+      }
+
   };
 
+  namespace detail
+  {
+
+
+    template <class Facade1, class Facade2>
+    bool equal(Facade1 const& f1, Facade2 const& f2)
+    {
+      return iterator_core_access::equal_fwd(f1, f2);
+    }
+
+    template <class Facade1, class Facade2>
+    typename Facade1::difference_type distance_to(Facade1 const& f1, Facade2 const& f2)
+    {
+      return iterator_core_access::distance_to_fwd(f1, f2);
+    }
+
+  }
+
   //
   // iterator_facade - use as a public base class for defining new
   // standard-conforming iterators.
@@ -550,19 +652,25 @@ namespace boost
   BOOST_ITERATOR_FACADE_RELATION(>=, return 0 <=, distance_to)
 # undef BOOST_ITERATOR_FACADE_RELATION
 
-  // operator- requires an additional part in the static assertion
+//   // operator- requires an additional part in the static assertion
-  BOOST_ITERATOR_FACADE_INTEROP(
+//   BOOST_ITERATOR_FACADE_INTEROP(
-      -
+//       -
-    , typename Derived1::difference_type
+//     , typename Derived1::difference_type
-    , (is_same<
+//     , (is_same<
-           BOOST_DEDUCED_TYPENAME Derived1::difference_type
+//            BOOST_DEDUCED_TYPENAME Derived1::difference_type
-         , BOOST_DEDUCED_TYPENAME Derived2::difference_type
+//          , BOOST_DEDUCED_TYPENAME Derived2::difference_type
-       >::value)
+//        >::value)
-    , return
+//     , return
-    , distance_to )
+  //    , distance_to )
 # undef BOOST_ITERATOR_FACADE_INTEROP
 # undef BOOST_ITERATOR_FACADE_INTEROP_HEAD
 
+    template < class Derived1, class V1, class TC1, class R1, class D1 , class Derived2, class V2, class TC2, class R2, class D2 > typename detail::enable_if_interoperable< Derived1, Derived2, typename std::iterator_traits<typename detail::interoperable_base<Derived1, Derived2> ::type > ::difference_type > ::type operator -( iterator_facade<Derived1, V1, TC1, R1, D1> const& lhs , iterator_facade<Derived2, V2, TC2, R2, D2> const& rhs)
+  {
+    return iterator_core_access::distance_to(static_cast<Derived2 const&>(rhs), static_cast<Derived1 const&>(lhs));
+  }
+
+
 # define BOOST_ITERATOR_FACADE_PLUS(args)           \
   BOOST_ITERATOR_FACADE_PLUS_HEAD(inline, args)     \
   {                                                 \