Re: [boost] Proposed interface change to boost::algorithm::copy_while

Subject: Re: [boost] Proposed interface change to boost::algorithm::copy_while
From: Neil Groves (neil_at_[hidden])
Date: 2013-02-20 16:05:19

On Wed, Feb 20, 2013 at 6:55 PM, Marshall Clow <mclow.lists_at_[hidden]>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/algorithms/introduction.html

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