Re: [PATCH] Specialize string::replace (Take2)

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

I have reviewed the patch, it looks good to me.
Mainline and branch.

Nathan Myers
ncm at cantrip dot org


On Fri, Mar 29, 2002 at 04:36:31PM +0100, Paolo Carlini wrote:
> this is the second take, where I have incorporated to my best Philip's 
> and Nathan's (privately expressed) comments:
> 
> - For Nathan: as you can see, this version avoids /completely/ any form 
> of cast (of course, no free lunch: some still happen but only behind the 
> scenes, in the form of implicit basic_string::_M_rep calls ;) Do you 
> like it?
> 
> - For Philip: I think I have implemented your hints about const-ness and 
> base(). Ok with you?
> 
> Some tests added, tested i686-pc-linux-gnu.
> 
> Ciao, Paolo.
> 
> //////////////////
> 
> 2002-03-29  Paolo Carlini  <pcarlini@unitus.it>
>             Nathan Myers  <ncm@cantrip.org>
>             Philip Martin  <philip@codematters.co.uk>     
>        
>         * include/bits/basic_string.h
>         (replace(i1, i2, _CharT* k1, _CharT* k2),
>         replace(i2, i2, const _CharT* k1, const _CharT* k2),
>         replace(i1, i2, iterator k1, iterator k2,
>         replace(i1, i2, const_iterator k1, const_iterator k2):
>         New specializations to optimize for the common cases of
>         pointers and iterators.
>         (replace(pos, n1, s, n2)): Tweak.
>         * include/bits/basic_string.tcc: Tweak comments.
>         * testsuite/21_strings/replace.cc (test04): New tests.
> 
> diff -urN libstdc++-v3-orig/include/bits/basic_string.h 
> libstdc++-v3/include/bits/basic_string.h
> --- libstdc++-v3-orig/include/bits/basic_string.h    Wed Mar  6 22:22:52 
> 2002
> +++ libstdc++-v3/include/bits/basic_string.h    Fri Mar 29 16:08:18 2002
> @@ -647,8 +647,8 @@
>          || less<const _CharT*>()(_M_data() + __size, __s))
>        return _M_replace_safe(_M_ibegin() + __pos,
>                   _M_ibegin() + __pos + __foldn1, __s, __s + __n2);   
> -    else return this->replace(_M_check(__pos), _M_fold(__pos, __n1),
> -                  __s, __s + __n2);
> +    else return replace(_M_check(__pos), _M_fold(__pos, __n1),
> +                basic_string(__s, __s + __n2));
>        }
>  
>        basic_string&
> @@ -682,6 +682,28 @@
>          { return _M_replace(__i1, __i2, __k1, __k2,
>           typename iterator_traits<_InputIterator>::iterator_category()); }
>  
> +      basic_string&
> +      replace(iterator __i1, iterator __i2, _CharT* __k1, _CharT* __k2)
> +        { return this->replace(__i1 - _M_ibegin(), __i2 - __i1,
> +                   __k1, __k2 - __k1); }
> +
> +      basic_string&
> +      replace(iterator __i1, iterator __i2, const _CharT* __k1, const 
> _CharT* __k2)
> +        { return this->replace(__i1 - _M_ibegin(), __i2 - __i1,
> +                   __k1, __k2 - __k1); }
> +
> +      basic_string&
> +      replace(iterator __i1, iterator __i2, iterator __k1, iterator __k2)
> +        { return this->replace(__i1 - _M_ibegin(), __i2 - __i1,
> +                   __k1.base(), __k2 - __k1);
> +    }
> +
> +      basic_string&
> +      replace(iterator __i1, iterator __i2, const_iterator __k1, 
> const_iterator __k2)
> +        { return this->replace(__i1 - _M_ibegin(), __i2 - __i1,
> +                   __k1.base(), __k2 - __k1);
> +    }
> +
>      private:
>        template<class _InputIterator>
>          basic_string&
> @@ -690,8 +712,8 @@
>  
>        template<class _ForwardIterator>
>          basic_string&
> -        _M_replace_safe(iterator __i1, iterator __i2, _ForwardIterator 
> __k1,
> -           _ForwardIterator __k2);
> +        _M_replace_safe(iterator __i1, iterator __i2, _ForwardIterator 
> __k1,
> +            _ForwardIterator __k2);
>  
>        // _S_construct_aux is used to implement the 21.3.1 para 15 which
>        // requires special behaviour if _InIter is an integral type
> diff -urN libstdc++-v3-orig/include/bits/basic_string.tcc 
> libstdc++-v3/include/bits/basic_string.tcc
> --- libstdc++-v3-orig/include/bits/basic_string.tcc    Tue Mar 12 
> 23:10:33 2002
> +++ libstdc++-v3/include/bits/basic_string.tcc    Fri Mar 29 15:26:41 2002
> @@ -498,13 +498,6 @@
>        // else nothing (in particular, avoid calling _M_mutate() 
> unnecessarily.)
>      }
>   
> -  // This is the general replace helper, which gets instantiated both
> -  // for input-iterators and forward-iterators. It buffers internally and
> -  // then calls _M_replace_safe. For input-iterators this is almost the
> -  // best we can do, but for forward-iterators many optimizations could be
> -  // conceived: f.i., when source and destination ranges do not overlap
> -  // buffering is not really needed. In order to easily implement them, it
> -  // could become useful to add an _M_replace(forward_iterator_tag)
>    template<typename _CharT, typename _Traits, typename _Alloc>
>      template<typename _InputIter>
>        basic_string<_CharT, _Traits, _Alloc>&
> @@ -518,10 +511,8 @@
>        }
>  
>    // This is a special replace helper, which does not buffer internally
> -  // and can be used in the "safe" situations involving forward-iterators,
> +  // and can be used in "safe" situations involving forward-iterators,
>    // i.e., when source and destination ranges are known to not overlap.
> -  // Presently, is called by _M_replace, by the various append and by
> -  // the assigns.
>    template<typename _CharT, typename _Traits, typename _Alloc>
>      template<typename _ForwardIter>
>        basic_string<_CharT, _Traits, _Alloc>&
> diff -urN libstdc++-v3-orig/testsuite/21_strings/replace.cc 
> libstdc++-v3/testsuite/21_strings/replace.cc
> --- libstdc++-v3-orig/testsuite/21_strings/replace.cc    Mon Jan 14 
> 20:04:15 2002
> +++ libstdc++-v3/testsuite/21_strings/replace.cc    Fri Mar 29 15:24:57 2002
> @@ -139,10 +139,47 @@
>    VERIFY(str01 == "ultra");
>  }
>  
> +// Some more tests for
> +// template<typename InputIter>
> +//   string& replace(iterator it1, iterator it2, InputIter j1, 
> InputIter j2)
> +void
> +test04()
> +{
> +  std::string str01 = "geogaddi";
> +  std::string str02;
> +
> +  typedef std::string::iterator iterator;
> +  typedef std::string::const_iterator const_iterator;
> + 
> +  iterator it1 = str01.begin();
> +  iterator it2 = str01.end();
> +  str02.replace(str02.begin(), str02.end(), it1, it2);
> +  VERIFY(str02 == "geogaddi");
> +
> +  str02 = "";
> +  const_iterator c_it1 = str01.begin();
> +  const_iterator c_it2 = str01.end();
> +  str02.replace(str02.begin(), str02.end(), c_it1, c_it2);
> +  VERIFY(str02 == "geogaddi");
> +
> +  str02 = "";
> +  const char* c_ptr1 = str01.c_str();
> +  const char* c_ptr2 = str01.c_str() + 8;
> +  str02.replace(str02.begin(), str02.end(), c_ptr1, c_ptr2);
> +  VERIFY(str02 == "geogaddi");
> +
> +  str02 = "";
> +  char* ptr1 = &*str01.begin();
> +  char* ptr2 = &*str01.end();
> +  str02.replace(str02.begin(), str02.end(), ptr1, ptr2);
> +  VERIFY(str02 == "geogaddi");
> +}
> +
>  int main()
>  {
>    test01();
>    test02();
>    test03();
> +  test04();
>    return 0;
>  }