Proposed interface change to boost::algorithm::copy_while
In 1.50, I introduced: template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p ); (and copy_until), which do pretty much what you would expect: copy items from the input to the output as long as the predicate holds. Sean Parent has convinced me that this is wrong; that I have to return the modified input iterator as well. When I wrote these, I was thinking (if I actually was thinking) of actual input iterators, and since they're all the same, so the caller can pick up on the input sequence after the call returns. However, if you're processing (say) a list, it would be kind of useful to know how far in the list the call advanced. I'm proposing to change the interface to: template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p ); i.e, changing the return type to return both iterators. What this means: * If you're not calling copy_while (or copy_until), then this change won't affect you. * If you're not using the return value, then this change won't affect you. * If you are using the return value, then you will have to change your code thus: Old foo = copy_while ( first, last, out, p ); New: foo = copy_while ( first, last, out, p ).second; * if you were not using these calls because they didn't return the input iterator, now you can. Questions? Comments? (Except for the "How could you miss that when you wrote those routines?" - I've already asked myself that) Improvements? -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On 20.02.2013, at 19:55, Marshall Clow wrote:
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
What this means: * If you're not calling copy_while (or copy_until), then this change won't affect you. * If you're not using the return value, then this change won't affect you. * If you are using the return value, then you will have to change your code thus: Old foo = copy_while ( first, last, out, p ); New: foo = copy_while ( first, last, out, p ).second;
If you want to ease transition, I once wrote a biased_pair type, which is basically a pair with an implicit conversion to one of its member types. template <typename T1, typename T2> struct right_biased_pair { // consider deriving from std::pair instead of aping it T1 first; T2 second; // constructors and stuff operator std::pair<T1, T2>() { return std::pair<T1, T2>(first, second); } // the bias operator T2() const { return second; } }; Then you can make copy_while return a right_biased_pair<InputIterator, OutputIterator> and code of the form out = copy_while(first, last, out, p); will keep working. No idea if that's worth it to you, just saying that the option is there. Sebastian
On Wed, Feb 20, 2013 at 6:55 PM, Marshall Clow <mclow.lists@gmail.com>wrote:
In 1.50, I introduced:
template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
(and copy_until), which do pretty much what you would expect: copy items from the input to the output as long as the predicate holds.
Sean Parent has convinced me that this is wrong; that I have to return the modified input iterator as well. When I wrote these, I was thinking (if I actually was thinking) of actual input iterators, and since they're all the same, so the caller can pick up on the input sequence after the call returns. However, if you're processing (say) a list, it would be kind of useful to know how far in the list the call advanced.
Ah yes, Sean is indeed a wise man. It's good to know his input is still available.
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!) I had similar agonising moments with the range return types of many algorithms and eventually decided that I would provide a template parameter that the user could select to choose the return type. The boost::unique algorithm is one example of such an affected function. See http://www.boost.org/doc/libs/1_53_0/libs/range/doc/html/range/reference/alg... Of course, this might be over-the-top and perhaps it is better to provide no such option. This judgement I leave in your capable hands. It is another option that could potentially provide backward compatibility.
What this means: * If you're not calling copy_while (or copy_until), then this change won't affect you. * If you're not using the return value, then this change won't affect you. * If you are using the return value, then you will have to change your code thus: Old foo = copy_while ( first, last, out, p ); New: foo = copy_while ( first, last, out, p ).second; * if you were not using these calls because they didn't return the input iterator, now you can.
Questions? Comments? (Except for the "How could you miss that when you wrote those routines?" - I've already asked myself that) Improvements?
My comments are above, but ultimately I think you are on the right track and that the change will not be a painful one.
-- Marshall
Thanks for your work, Neil Groves
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!)
An alternative would be to take the 'first' iterator by reference. Boost.Spirit uses this approach. Regards, Nate
On Feb 20, 2013, at 4:05 PM, Nathan Ridge <zeratul976@hotmail.com> wrote:
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!)
An alternative would be to take the 'first' iterator by reference. Boost.Spirit uses this approach.
I've thought about that (and actually used it internally in boost::hex). However, for a public interface, I don't like it, because (among other things) it makes multi-threaded code harder to write. -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On Feb 20, 2013, at 4:05 PM, Nathan Ridge <zeratul976@hotmail.com> wrote:
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!)
An alternative would be to take the 'first' iterator by reference. Boost.Spirit uses this approach.
I just realized another reason not to like this. If your prototype looks like this: template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator &first, InputIterator last, OutputIterator result, Predicate ); you can't call it like this: copy_while ( v.begin (), v.end (), out, pred ); -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On 26 February 2013 20:36, Marshall Clow wrote:
I just realized another reason not to like this. If your prototype looks like this: template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator &first, InputIterator last, OutputIterator result, Predicate );
you can't call it like this: copy_while ( v.begin (), v.end (), out, pred );
But you could add the by-reference form as an overload, existing code passing an rvalue gets the old behaviour and doesn't get the extra information, which is OK because they never got it before now anyway.
On Feb 26, 2013, at 12:50 PM, Jonathan Wakely <jwakely.boost@kayari.org> wrote:
On 26 February 2013 20:36, Marshall Clow wrote:
I just realized another reason not to like this. If your prototype looks like this: template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator &first, InputIterator last, OutputIterator result, Predicate );
you can't call it like this: copy_while ( v.begin (), v.end (), out, pred );
But you could add the by-reference form as an overload, existing code passing an rvalue gets the old behaviour and doesn't get the extra information, which is OK because they never got it before now anyway.
Yes, but existing callers who were passing an lvalue get a silent change in behavior; their variable changes out from under them. -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On 13-02-26 01:00 PM, Marshall Clow wrote:
On Feb 26, 2013, at 12:50 PM, Jonathan Wakely <jwakely.boost@kayari.org> wrote:
On 26 February 2013 20:36, Marshall Clow wrote:
I just realized another reason not to like this. If your prototype looks like this: template<typename InputIterator, typename OutputIterator, typename Predicate> OutputIterator copy_while ( InputIterator &first, InputIterator last, OutputIterator result, Predicate );
you can't call it like this: copy_while ( v.begin (), v.end (), out, pred );
But you could add the by-reference form as an overload, existing code passing an rvalue gets the old behaviour and doesn't get the extra information, which is OK because they never got it before now anyway.
Yes, but existing callers who were passing an lvalue get a silent change in behavior; their variable changes out from under them.
There is no precedent in the standard library of passing an iterator by reference to an algorithm and having the algorithm mutate it. In/out parameters are not in keeping with the functional style of the STL. IMO, this would be a very poor choice for the interface. Very error prone, very surprising. -- Eric Niebler Boost.org
On Feb 20, 2013, at 1:05 PM, Neil Groves <neil@grovescomputing.com> wrote:
On Wed, Feb 20, 2013 at 6:55 PM, Marshall Clow <mclow.lists@gmail.com>wrote:
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!)
Whoa. That's something I didn't know about. Boost.Range will take std::pair<Iter, Iter> as a range? Off to do more reading. ;-)
I had similar agonising moments with the range return types of many algorithms and eventually decided that I would provide a template parameter that the user could select to choose the return type. The boost::unique algorithm is one example of such an affected function. See http://www.boost.org/doc/libs/1_53_0/libs/range/doc/html/range/reference/alg...
Of course, this might be over-the-top and perhaps it is better to provide no such option. This judgement I leave in your capable hands. It is another option that could potentially provide backward compatibility.
What this means: * If you're not calling copy_while (or copy_until), then this change won't affect you. * If you're not using the return value, then this change won't affect you. * If you are using the return value, then you will have to change your code thus: Old foo = copy_while ( first, last, out, p ); New: foo = copy_while ( first, last, out, p ).second; * if you were not using these calls because they didn't return the input iterator, now you can.
Questions? Comments? (Except for the "How could you miss that when you wrote those routines?" - I've already asked myself that) Improvements?
My comments are above, but ultimately I think you are on the right track and that the change will not be a painful one.
Thanks for the feedback. I've checked in the proposed change (and tests!) as revision 83064. More comments welcomed. -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
On 2/21/2013 11:23 AM, Marshall Clow wrote:
On Feb 20, 2013, at 1:05 PM, Neil Groves <neil@grovescomputing.com> wrote:
On Wed, Feb 20, 2013 at 6:55 PM, Marshall Clow <mclow.lists@gmail.com>wrote:
I'm proposing to change the interface to:
template<typename InputIterator, typename OutputIterator, typename Predicate> std::pair<InputIterator, OutputIterator> copy_while ( InputIterator first, InputIterator last, OutputIterator result, Predicate p );
i.e, changing the return type to return both iterators.
I think that certainly a change needs to be made to accomodate the extra information. I wonder how wise it is to return a pair that when the InputIterator type is the same as the OutputIterator type that it could be mistaken for, and accepted by, range algorithms as input? (That's my fault - doh!)
Whoa. That's something I didn't know about. Boost.Range will take std::pair<Iter, Iter> as a range? Off to do more reading. ;-)
FYI, std::mismatch returns a pair of possibly unrelated iterators, template <class InputIterator1, class InputIterator2> pair<InputIterator1, InputIterator2> mismatch (InputIterator1 first1, InputIterator1 last1, InputIterator2 first2 ) In the case of std::mismatch both iterators have an arguably higher probability to be the same type. Jeff
participants (7)
-
Eric Niebler -
Jeff Flinn -
Jonathan Wakely -
Marshall Clow -
Nathan Ridge -
Neil Groves -
Sebastian Redl