Re: [PATCH] Basic_string::_M_replace clean up

[Nathan Myers <ncm-nospam at cantrip dot org>]

On Sat, Dec 08, 2001 at 10:14:08PM +0100, Paolo Carlini wrote:
> Currently, we have the following helper
> 
>   basic_string::_M_replace(iterator __i1, iterator __i2, _InputIter __k1,
>                            _InputIter __k2, input_iterator_tag)
> in addition to
> 
>   basic_string::_M_replace(iterator __i1, iterator __i2, _ForwardIter __k1,
>                            _ForwardIter __k2, forward_iterator_tag)
> 
> What I propose is getting rid of the first, changing the signature of
> the second to:
> 
>   basic_string::_M_replace(iterator __i1, iterator __i2, _InputIter __k1,
>                                _InputIter __k2, input_iterator_tag)

The point of having the two is that for the forward_iterator_tag version
you can count the number of elements to insert, allocate the space once,
and then copy them into place.  An input iterator cannot be re-read,
so there's no better way than to allocate as you go.  Therefore, it is
necessary to keep the distinction.

The call using _M_ibegin() and _M_iend() instantiates a version that 
takes random-access iterators, and which can determine the amount of 
space needed by simple subtraction.  It appears, in fact, to call the 
forward iterator version, with the distance() function expanding to do 
the right thing for the iterators involved -- looping for actual 
forward iterators, and subtracting for random access iterators (in
this case, pointers).
 
> The former appears completely unused (I have simply checked for this
> by running the testsuite with a throw in it) and in fact, if it were,
> it would call:
> 
>       replace(__i1, __i2, __s._M_ibegin(), __s._M_iend())
> 
> which has a signature (iterator, iterator, iterator, iterator) and thus:
> 
>       template<class _InputIterator>
>         basic_string&
>         replace(iterator __i1, iterator __i2, _InputIterator __k1,
> _InputIterator __k2)
>         {
>           return _M_replace(__i1, __i2, __k1, __k2,
>               typename iterator_traits<_InputIterator>::iterator_category());
>         }
> 
> replace in principle could either call _M_replace(input_iterator_tag), thus
> leading to infinite recursion, or _M_replace(forward_iterator_tag), with a
> definite loss of efficiency.

In fact, it would call the second.  The second form would not work 
correctly on a real input iterator.  I don't see the loss of efficiency.
If it were inefficient, the correct approach would be to add a version
that takes a random_access_iterator_tag.

If the first is not called in the test suite, that is an omission from 
the test suite that should be fixed.  You can use a stringstream and
a pair of istreambuf_iterators or istream_iterators.

Nathan Myers
ncm at cantrip dot org