Re: [patch] Simplify non-inline function definitions for std::unordered_xxx containers

[Jonathan Wakely <jwakely at redhat dot com>]

On 04/12/14 11:39 +0000, Jonathan Wakely wrote:
> On 03/12/14 23:32 +0100, François Dumont wrote:
> > On 03/12/2014 16:59, Jonathan Wakely wrote:
> > > François (or anyone else), do you see any problem with this change?
> > >
> > > It makes the code shorter and I think is much easier to read, it also
> > > reduces the memory total shown by -ftime-report.
> > No, no problem. Interesting to see what benefit we can have when 
> > reducing number of template parameters. I will try to recall it in 
> > the future.
>
> We could remove the _Value parameter from every class template that
> also takes _Alloc, because we can get it back from _Alloc::value_type
> (assuming we consistently rebind the allocator to the value_type
> before instantiating _Hashtable, which we don't currently do).
>
> I have a patch coming soon for PR57272 that might include a change
> like that, because I already need to replace _Value with
> allocator_traits<_Alloc>::pointer everywhere.

i.e. like this. Please take a look and let me know what you think.

Although this touches almost every line of the hashtable.h and
hastable_policy.h files, it's mostly mechanical. The main purpose is
to replace every use of X* with a typedef like X_pointer, which comes
from the allocator. In the common case it's just a typedef for X*, but
the allocator can use something else. This fixes PR57272.

To make that work the _Hash_node_base and related types need to be
parameterised on the allocator's pointer, and then be fixed to not
rely on implicit conversions from pointer-to-derived to
pointer-to-base (see
http://cplusplus.github.io/LWG/lwg-active.html#2260). Once that change
is done it makes sense to replace the _Value parameter everywhere,
deriving it from the allocator instead.

(I can't help thinking the unordered container code would be much
easier to read if we just had a bundle of all the relevant template
parameters and passed that around, instead of passing the same 9
parameters to every class template!)

Something like the attached patch is necessary to fix PR57272,
although I also have an earlier version that just adds the _Ptr
parameter to the end of the template parameter list, instead of
removing _Value.  We should probably do it for GCC 5.0 if we're ever
going to do it. I have a similar patch for forward_list as well.

There is an alternative to refactoring everything to use the pointer
type, which I'm doing for std::list, std::map etc. to be
backward-compatible. That alternative is to keep the existing
non-template _Hash_node_base type but add an alternative
_Hash_node_ptr_base<_VoidPtr> which is used when the allocator has
"fancy pointers". That is extremely painful, because you need an
alternative _Hash_node<_Ptr> and _Node_iterator<_Ptr> and
_Local_iterator<_Ptr> and so on for every related type.

P.S. this patch also rewrites the __gnu_test::CustomPointerAlloc test
allocator to use a rewritten __gnu_test::CustomPointer which matches
the C++11 requirements better than __gnu_cxx::_Pointer_adapter does.
It also uses the __allocated_obj RAII type to manage
creating/destroying nodes safely without needing try/catch. Both those
changes have been very useful for all the containers when testing and
fixing PR57272.

Attachment: patch.txt.bz2
Description: BZip2 compressed data