Re: [patch/rfa] Tiny but delicate change to string::_M_mutate

[Paolo Carlini <pcarlini at suse dot de>]

Nathan Myers wrote:

> On Fri, Oct 15, 2004 at 08:26:18PM +0200, Paolo Carlini wrote:
>  
>
> > Hi everyone, hi Nathan,
> >    
> Apologies for the delay in replying...
>  
No problem, 4.0.0 is still a few months away :) ...

> This looks perfectly safe.  It seems probable that people often do
> append no characters, e.g. because the string they are appending
> is empty.  Probably the logic should still check for _S_empty_rep
> to avoid touching its reference count:
>
>     if ((((_M_rep() == &_S_empty_rep() || _M_rep()->_M_is_shared()) 
>            && (__len2 | __len1) != 0) 
> 	  || __new_size > capacity())
>  
Why this complication? The conditional becomes *awful* (sorry ;) and we have
to keep the macro... What's wrong with reconfirming (in the *very few* 
cases the
codepath at issue typically triggers) the refcount to zero? (*) If we 
really want to
avoid that, I propose simply changing the end of _M_mutate to:

#ifndef _GLIBCXX_FULLY_DYNAMIC_STRING
   if (_M_rep() != &_S_empty_rep()
#endif
       {
         _M_rep()->_M_set_sharable();
         _M_rep()->_M_length = __new_size;
         _M_data()[__new_size] = _Rep::_S_terminal; // grrr. (per 21.3.4)
       }

But I would really prefer not adding this... 

> We might instead check that the proposed mutation actually changes 
> the string before calling _M_mutate; that would subsume the case of 
> appending the empty string, and make the present logic correct   
> (...if I understand correctly).
>  
This is an additional improvement worth looking into, thanks! I suspect
we gain a lot of performance for simple operations (i.e., single char
appends) if we avoid involving the complete _M_mutate...

Thanks Nathan,
Paolo.

(*) Ok, it's obvious, sorry, but _M_set_sharable, is just an inlined 
assignment
of an integer.