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: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.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.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.
patch.txt.bz2
Description: BZip2 compressed data