[Range] patched: boost/range/iterator.hpp
Hi Thorsten, I took the liberty of checking this patch into the HEAD: Please let me know if you have any objections. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
> Hi Thorsten,
>
> I took the liberty of checking this patch into the HEAD:
+ template< typename C >
+ struct range_iterator<C const>
+ {
+ typedef BOOST_DEDUCED_TYPENAME C::const_iterator type;
+ };
+
So, I no longer need to use range_const_iterator?
Neal Becker <ndbecker2@gmail.com> writes:
> David Abrahams wrote:
>
>> Hi Thorsten,
>>
>> I took the liberty of checking this patch into the HEAD:
> + template< typename C >
> + struct range_iterator<C const>
> + {
> + typedef BOOST_DEDUCED_TYPENAME C::const_iterator type;
> + };
> +
>
> So, I no longer need to use range_const_iterator?
Assuming Thorsten likes the patch, that's right.
--
Dave Abrahams
Boost Consulting
www.boost-consulting.com
David Abrahams wrote:
Hi Thorsten,
I took the liberty of checking this patch into the HEAD:
------------------------------------------------------------------------
Index: iterator.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/range/iterator.hpp,v retrieving revision 1.4 retrieving revision 1.5 diff -b -d -u -b -u -r1.4 -r1.5 --- iterator.hpp 5 Jan 2005 18:19:31 -0000 1.4 +++ iterator.hpp 8 May 2006 10:21:50 -0000 1.5 @@ -37,6 +37,12 @@ typedef BOOST_DEDUCED_TYPENAME C::iterator type; };
+ template< typename C > + struct range_iterator<C const> + { + typedef BOOST_DEDUCED_TYPENAME C::const_iterator type; + }; + ////////////////////////////////////////////////////////////////////////// // pair //////////////////////////////////////////////////////////////////////////
------------------------------------------------------------------------
Please let me know if you have any objections.
I object. In the current range interface, the meaning of the range metafunctions are as follows: range_iterator: always return ::iterator range_const_iterator: always return ::const_iterator range_result_iterator: return either ::iterator or ::const_iterator depending on the const-ness of the argument. This was deemed an unfortunate design, and there is a reworking of the Range interface in the pipeline. Thorsten wanted to apply it for 1.34, but it was too late in the process. Your patch gets everything all mixed up. My suggestion is that you back this change out and Thorsten applies his changes to HEAD. -- Eric Niebler Boost Consulting www.boost-consulting.com
"Eric Niebler" <eric@boost-consulting.com> writes:
David Abrahams wrote:
Hi Thorsten,
I took the liberty of checking this patch into the HEAD:
------------------------------------------------------------------------
Index: iterator.hpp =================================================================== RCS file: /cvsroot/boost/boost/boost/range/iterator.hpp,v retrieving revision 1.4 retrieving revision 1.5 diff -b -d -u -b -u -r1.4 -r1.5 --- iterator.hpp 5 Jan 2005 18:19:31 -0000 1.4 +++ iterator.hpp 8 May 2006 10:21:50 -0000 1.5 @@ -37,6 +37,12 @@ typedef BOOST_DEDUCED_TYPENAME C::iterator type; };
+ template< typename C > + struct range_iterator<C const> + { + typedef BOOST_DEDUCED_TYPENAME C::const_iterator type; + }; + ////////////////////////////////////////////////////////////////////////// // pair //////////////////////////////////////////////////////////////////////////
------------------------------------------------------------------------
Please let me know if you have any objections.
I object. In the current range interface, the meaning of the range metafunctions are as follows:
range_iterator: always return ::iterator range_const_iterator: always return ::const_iterator range_result_iterator: return either ::iterator or ::const_iterator depending on the const-ness of the argument.
Ugh, I forgot about the existence of range_result_iterator.
This was deemed an unfortunate design, and there is a reworking of the Range interface in the pipeline. Thorsten wanted to apply it for 1.34, but it was too late in the process.
Your patch gets everything all mixed up.
That's rather a bald and sweeping statement to make without any explanation. What do you really mean by "everything" in this case? AFAICT it's entirely consistent. Look: typedef std::vector<int> vec; vec v; vec const& vc; boost::begin(v) -> vec::iterator -> range_iterator<typeof(v)>::type boost::begin(vc) -> vec::const_iterator -> range_iterator<typeof(v)>::type boost::const_begin(v) -> vec::const_iterator -> range_const_iterator<typeof(v)>::type boost::const_begin(vc) -> vec::const_iterator -> range_const_iterator<typeof(v)>::type That is, the result type of begin is always expressed by range_iterator and the result type of const_begin is always expressed by range_const_iterator. Perfectly symmetric and consistent. Before my change, the best you could say was that the result type of begin is always expressed by range_result_iterator and the result type of const_begin is always expressed by range_const_iterator.
My suggestion is that you back this change out and Thorsten applies his changes to HEAD.
Why? What other changes do you think are needed in order to make this consistent? Anyway, now that you've reminded me of range_result_iterator, I'll be happy to roll back my change if this is breaking any code or if Thorsten asks me to. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Eric Niebler" <eric@boost-consulting.com> writes:
David Abrahams wrote:
Hi Thorsten,
I took the liberty of checking this patch into the HEAD:
<snip>
Please let me know if you have any objections.
I object. In the current range interface, the meaning of the range metafunctions are as follows:
range_iterator: always return ::iterator range_const_iterator: always return ::const_iterator range_result_iterator: return either ::iterator or ::const_iterator depending on the const-ness of the argument.
Ugh, I forgot about the existence of range_result_iterator.
This was deemed an unfortunate design, and there is a reworking of the Range interface in the pipeline. Thorsten wanted to apply it for 1.34, but it was too late in the process.
Your patch gets everything all mixed up.
That's rather a bald and sweeping statement to make without any explanation. What do you really mean by "everything" in this case?
Everything, in this case, means the separate and orthogonal responsibilities of the three range metafunctions. This is a potentially breaking change. Consider a range adaptor... template<typename Range> struct some_adaptor { typedef typename range_iterator<Range>::type iterator; typedef typename range_const_iterator<Range>::type const_iterator; ... }; In this case, you really don't care about the const-ness of Range -- you just want the nested ::iterator type. And anyplace someone is using these metafunctions to perform type computations where const-ness doesn't matter will now be broken.
My suggestion is that you back this change out and Thorsten applies his changes to HEAD.
Why? What other changes do you think are needed in order to make this consistent?
Because your change is unnecessary (just use range_result_iterator) and because it could potentially break code.
Anyway, now that you've reminded me of range_result_iterator, I'll be happy to roll back my change if this is breaking any code or if Thorsten asks me to.
I also object to patching someone else's code without permission unless that person is unreachable, which Thorsten is not. Had you checked first, he (or I) would have simply told you to use range_result_iterator. -- Eric Niebler Boost Consulting www.boost-consulting.com
"Eric Niebler" <eric@boost-consulting.com> writes:
Why? What other changes do you think are needed in order to make this consistent?
Because your change is unnecessary (just use range_result_iterator) and because it could potentially break code.
Anyway, now that you've reminded me of range_result_iterator, I'll be happy to roll back my change if this is breaking any code or if Thorsten asks me to.
I also object to patching someone else's code without permission unless that person is unreachable, which Thorsten is not. Had you checked first, he (or I) would have simply told you to use range_result_iterator.
Okay, you're right. My only excuse is a poor one, so I won't even bother ;-) Un-patching... -- Dave Abrahams Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
David Abrahams wrote:
Your patch gets everything all mixed up.
That's rather a bald and sweeping statement to make without any explanation. What do you really mean by "everything" in this case?
Everything, in this case, means the separate and orthogonal responsibilities of the three range metafunctions.
This is a potentially breaking change. Consider a range adaptor...
template<typename Range> struct some_adaptor { typedef typename range_iterator<Range>::type iterator; typedef typename range_const_iterator<Range>::type const_iterator; .... };
In this case, you really don't care about the const-ness of Range -- you just want the nested ::iterator type. And anyplace someone is using these metafunctions to perform type computations where const-ness doesn't matter will now be broken.
In the new version, this is how range_iterator is defined: namespace boost { template< typename C > struct range_iterator { typedef BOOST_RANGE_DEDUCED_TYPENAME mpl::if_< BOOST_DEDUCED_TYPENAME is_const<C>::type, BOOST_DEDUCED_TYPENAME range_const_iterator< BOOST_DEDUCED_TYPENAME remove_const<C>::type
::type, BOOST_DEDUCED_TYPENAME range_mutable_iterator<C>::type >:: type type; };
} // namespace boost -Thorsten
Thorsten Ottosen <thorsten.ottosen@dezide.com> writes:
In the new version, this is how range_iterator is defined:
namespace boost { template< typename C > struct range_iterator { typedef BOOST_RANGE_DEDUCED_TYPENAME mpl::if_< BOOST_DEDUCED_TYPENAME is_const<C>::type, BOOST_DEDUCED_TYPENAME range_const_iterator< BOOST_DEDUCED_TYPENAME remove_const<C>::type
::type, BOOST_DEDUCED_TYPENAME range_mutable_iterator<C>::type >:: type type; };
} // namespace boost
Well, I think the logic is probably right but I have to say the code looks a lot more complex than it probably ought to. There are lots of places where the current code could profit from refactoring; this might be another such place. For example, if you make range_iterator do the right thing always, (maybe) you could spell range_const_iterator as follows: template <class T> struct range_const_iterator : range_iterator<T const> {}; It's just an example; you have to check it to make sure it works out. Anyway, I suggest you look hard for simplifying refactorings before you commit a big rewrite. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
Thorsten Ottosen <thorsten.ottosen@dezide.com> writes:
In the new version, this is how range_iterator is defined:
namespace boost { template< typename C > struct range_iterator { typedef BOOST_RANGE_DEDUCED_TYPENAME mpl::if_< BOOST_DEDUCED_TYPENAME is_const<C>::type, BOOST_DEDUCED_TYPENAME range_const_iterator< BOOST_DEDUCED_TYPENAME remove_const<C>::type >::type, BOOST_DEDUCED_TYPENAME range_mutable_iterator<C>::type >:: type type; };
} // namespace boost
Well, I think the logic is probably right but I have to say the code looks a lot more complex than it probably ought to.
I think I can remove some partial specializations based on constness, like you suggested, eg.: template< class R > const_iterator<R const> : const_iterator<R> { }; But besides that, what else do you think looks complicated? (Constness has certainly been a major pain)
There are lots of places where the current code could profit from refactoring; this might be another such place. For example, if you make range_iterator do the right thing always, (maybe) you could spell range_const_iterator as follows:
template <class T> struct range_const_iterator : range_iterator<T const> {};
It's just an example; you have to check it to make sure it works out. Anyway, I suggest you look hard for simplifying refactorings before you commit a big rewrite.
I the new design, range_iterator depends on two other traits: - range_mutable_iterator - range_const_iterator your suggestion would make the design circular dependent. -Thorsten
Thorsten Ottosen <thorsten.ottosen@dezide.com> writes:
David Abrahams wrote:
Thorsten Ottosen <thorsten.ottosen@dezide.com> writes:
In the new version, this is how range_iterator is defined:
namespace boost { template< typename C > struct range_iterator { typedef BOOST_RANGE_DEDUCED_TYPENAME mpl::if_< BOOST_DEDUCED_TYPENAME is_const<C>::type, BOOST_DEDUCED_TYPENAME range_const_iterator< BOOST_DEDUCED_TYPENAME remove_const<C>::type >::type, BOOST_DEDUCED_TYPENAME range_mutable_iterator<C>::type >:: type type; };
} // namespace boost
Well, I think the logic is probably right but I have to say the code looks a lot more complex than it probably ought to.
I think I can remove some partial specializations based on constness, like you suggested, eg.:
template< class R > const_iterator<R const> : const_iterator<R> { };
But besides that, what else do you think looks complicated?
Maybe it's just all the macros. BOOST_DEDUCED_TYPENAME should have been renamed BOOST_TYPENAME years ago, but you're still using it way more than I've ever found necessary to satisfy old compilers.
(Constness has certainly been a major pain)
There are lots of places where the current code could profit from refactoring; this might be another such place. For example, if you make range_iterator do the right thing always, (maybe) you could spell range_const_iterator as follows:
template <class T> struct range_const_iterator : range_iterator<T const> {};
It's just an example; you have to check it to make sure it works out. Anyway, I suggest you look hard for simplifying refactorings before you commit a big rewrite.
I the new design, range_iterator depends on two other traits:
- range_mutable_iterator - range_const_iterator
your suggestion would make the design circular dependent.
Well of course I wasn't suggesting circularity. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
Thorsten Ottosen <thorsten.ottosen@dezide.com> writes:
namespace boost { template< typename C > struct range_iterator { typedef BOOST_RANGE_DEDUCED_TYPENAME mpl::if_< BOOST_DEDUCED_TYPENAME is_const<C>::type, BOOST_DEDUCED_TYPENAME range_const_iterator< BOOST_DEDUCED_TYPENAME remove_const<C>::type >::type, BOOST_DEDUCED_TYPENAME range_mutable_iterator<C>::type >:: type type; };
} // namespace boost
Well, I think the logic is probably right but I have to say the code looks a lot more complex than it probably ought to.
But besides that, what else do you think looks complicated?
Maybe it's just all the macros. BOOST_DEDUCED_TYPENAME should have been renamed BOOST_TYPENAME years ago, but you're still using it way more than I've ever found necessary to satisfy old compilers.
Well, I think I also need to remove BOOST_RANGE_DEDUCED_TYPENAME. Maybe it's time for sed --in-place -e "s/BOOST_DEDUCED_TYPENAME/BOOST_TYPENAME/" . ? -Thorsten
David Abrahams wrote:
"Eric Niebler" <eric@boost-consulting.com> writes:
My suggestion is that you back this change out and Thorsten applies his changes to HEAD.
Why? What other changes do you think are needed in order to make this consistent?
Anyway, now that you've reminded me of range_result_iterator, I'll be happy to roll back my change if this is breaking any code or if Thorsten asks me to.
I have a long line of changes in the pipeline which, as Eric remembers, I couldn't commit before the branch for release. It does what you I think, in the sense that range_iterator behaves like range_result_iterator do today. -Thorsten
Thorsten Ottosen wrote:
I have a long line of changes in the pipeline which, as Eric remembers, I couldn't commit before the branch for release.
You should commit those changes to HEAD sooner rather than later, IMO. -- Eric Niebler Boost Consulting www.boost-consulting.com
participants (5)
-
David Abrahams -
Eric Niebler -
Neal Becker -
Thorsten Ottosen -
Thorsten Ottosen