Re: [Patch] libstdc++/23425

[Howard Hinnant <hhinnant at apple dot com>]

On Nov 1, 2005, at 8:53 AM, Paolo Carlini wrote:

> Hi,
>
> I think it makes perfect sense to apply the simple patch submitted as
> part of the PR: in case of clear() we know a lot more of a generic
> erase(iterator, iterator) and, indeed, "manually inlining" the current
> erase call and cleaning up the result we end up with the very same  
> code
> of ~vector() (+ adjustment for the _M_finish pointer). The impact  
> on the
> generated assembly is impressive (x86, -O2 -fomit-frame-pointer):

I don't have contrary opinion, just a note that we need more of this.

clear() is best not thought of as a special case of erase().  It is  
best thought of as a special case of a private function which I'll call:

void __erase_at_end(size_type n);

Erases the last n elements from the vector.  This is needed in the  
following public members of vector:

All forms of assign.
All forms of erase.
All forms of resize.
clear.

I haven't studied it thoroughly yet but because of the structure of  
this implementation of vector, the following variant might be  
slightly more efficient:

void __erase_from(T* p);

where everything in [p, end()) gets erased.

Given such a private helper, we can optimize it just as you show for  
clear, but then propagate that optimization around to all of these  
other places in vector too.  clear() then becomes a trivial inline:

void clear() {__erase_from(this->_M_impl._M_start);}

All that being said, I have no objection to the patch as is for now.   
This is just a note of a direction that I think would be good to take  
in the future.

On the issue of inlining, and I'm speaking from an standpoint of  
great ignorance here, it could well be that __erase_from is a  
function that we want inlined if it is optimized (i.e. when the  
destructor is trivial), and not inlined if it is not optimized.  We  
can overload __erase_from on tr1::has_trivial_destructor<T>::value,  
inlining for true and not for false.

Before we do that though we need to improve has_trivial_destructor  
with compiler knowledge about the type.  Right now it just rings true  
for scalars and that is far too restrictive for prime time use.

-Howard