From fb1fc6f909a0c66485efa40ea5ecf04826860d88 Mon Sep 17 00:00:00 2001 From: Thomas Witt Date: Tue, 6 Jan 2004 19:37:17 +0000 Subject: [PATCH] *** empty log message *** [SVN r21519] --- doc/interoperability-revisited.rst | 232 +++++++++++++++++++++ doc/iter-issue-list.rst | 27 ++- include/boost/iterator/iterator_facade.hpp | 140 +++++++++++-- 3 files changed, 378 insertions(+), 21 deletions(-) create mode 100755 doc/interoperability-revisited.rst diff --git a/doc/interoperability-revisited.rst b/doc/interoperability-revisited.rst new file mode 100755 index 0000000..6754b9c --- /dev/null +++ b/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 +struct Facade +{ +}; + +template +struct is_interoperable : + or_< + is_convertible + , is_convertible + > +{}; + +template< + class Derived1 + , class Derived2 +> +enable_if, bool> operator==( + Derived1 const& lhs + , Derived2 const& rhs +) +{ + return static_cast(lhs).equal_to(static_cast +{ + bool equal_to(Mutable const&); +}; + +struct Constant : Facade +{ + 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 +{ + template + enable_if || is_convertible, bool>::type equal_to(T const&); +}; + +struct Constant : Tag +{ + Constant(); + Constant(Constant const&); + Constant(Mutable const&); + + template + enable_if || is_convertible, 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 +{ + 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, bool> operator==( + Derived1 const& lhs + , Derived2 const& rhs +) +{ + typedef interoperable_base< + Derived1 + , Derived2 + >::type Base; + + return static_cast(lhs).equal_to(static_cast +enable_if, bool> operator==( + Derived1 const& lhs + , Derived2 const& rhs +) +{ + return static_cast(lhs).equal_to(static_cast +enable_if, bool> operator==( + Derived1 const& lhs + , Derived2 const& rhs +) +{ + return static_cast(rhs).equal_to(static_cast +{ + 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) \ No newline at end of file diff --git a/doc/iter-issue-list.rst b/doc/iter-issue-list.rst index 4ae639c..9148c73 100644 --- a/doc/iter-issue-list.rst +++ b/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’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’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 -N1541 51 - +knowing only its value_type. This predicate can be calculated even for old-style iterators (on 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::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 ========================================= diff --git a/include/boost/iterator/iterator_facade.hpp b/include/boost/iterator/iterator_facade.hpp index 79b2d3c..bdc7eb3 100644 --- a/include/boost/iterator/iterator_facade.hpp +++ b/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 + , Facade2 + , Facade1 + > + { + }; + + template + struct difference_type + { + typedef typename interoperable_base::type::difference_type type; + }; + + template + bool equal(Facade1 const&, Facade2 const&); + + template + typename Facade1::difference_type distance_to(Facade1 const&, Facade2 const&); + + template + struct facade_binary_operator + { + template + static bool equal(Facade1 const& lhs, + Facade2 const& rhs) + { + return ::boost::detail::equal(lhs, rhs); + } + + template + 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 + { + template + static bool equal(Facade1 const& lhs, + Facade2 const& rhs) + { + return ::boost::detail::equal(rhs, lhs); + } + + template + 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( - friend, -, typename Derived1::difference_type) - ; + 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 ::type > ::difference_type > ::type operator -( iterator_facade const& lhs , iterator_facade const& rhs); + +// BOOST_ITERATOR_FACADE_INTEROP_HEAD( +// friend, -, (typename std::iterator_traits::type >::difference_type)) +// ; BOOST_ITERATOR_FACADE_PLUS_HEAD( friend @@ -297,7 +365,7 @@ namespace boost template 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 @@ -307,17 +375,51 @@ namespace boost } template - static typename Facade1::difference_type distance_to( + static typename detail::difference_type::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 + static bool equal_fwd(Facade1 const& f1, Facade2 const& f2) + { + return f1.equal(f2); + } + + template + static typename Facade1::difference_type distance_to_fwd( + Facade1 const& f1, Facade2 const& f2) + { + return f1.distance_to(f2); + } + }; + namespace detail + { + + + template + bool equal(Facade1 const& f1, Facade2 const& f2) + { + return iterator_core_access::equal_fwd(f1, f2); + } + + template + 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 - BOOST_ITERATOR_FACADE_INTEROP( - - - , typename Derived1::difference_type - , (is_same< - BOOST_DEDUCED_TYPENAME Derived1::difference_type - , BOOST_DEDUCED_TYPENAME Derived2::difference_type - >::value) - , return - , distance_to ) +// // operator- requires an additional part in the static assertion +// BOOST_ITERATOR_FACADE_INTEROP( +// - +// , typename Derived1::difference_type +// , (is_same< +// BOOST_DEDUCED_TYPENAME Derived1::difference_type +// , BOOST_DEDUCED_TYPENAME Derived2::difference_type +// >::value) +// , return + // , 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 ::type > ::difference_type > ::type operator -( iterator_facade const& lhs , iterator_facade const& rhs) + { + return iterator_core_access::distance_to(static_cast(rhs), static_cast(lhs)); + } + + # define BOOST_ITERATOR_FACADE_PLUS(args) \ BOOST_ITERATOR_FACADE_PLUS_HEAD(inline, args) \ { \