[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In https://reviews.llvm.org/D49317#1180852, @ldionne wrote:

> After thinking about this for some more, I'm not sure this patch is worth doing in its current form. The minimal patch for allowing specializations of `allocator_traits` would be:
>
> 1. move the `__move_construct_forward` & friends functions from `allocator_traits` to private static member functions of `std::vector` (because they're only used in `std::vector` right now).
> 2. keep the SFINAE on the allocator and avoid encoding any `memcpy` decision at the call site.


FWLIW, I approve of (1) but not (2), for the previously stated reason that the optimal path is known only at the call-site; the callee doesn't have enough information to know whether memcpy is appropriate.  (But it sounds like Marshall doesn't want any memcpy happening at all, so maybe it's moot?)

> However, an even better alternative would be to look into adding an overload to `uninitialized_move` & friends that takes an allocator. We could then be clever in how this is implemented. The major benefit I see here is that there would be one common code path to optimize, as opposed to a `std::vector`-specific code path.

Yes, when I implemented https://github.com/Quuxplusone/from-scratch/, one of the many things I noticed was that none of the uninitialized_foo algorithms were useful out of the box; every one of them needed to be reimplemented to take an allocator parameter. (A.k.a., "scoped_allocator_adaptor is why we can't have nice things.")  However, as you point out, this is a long-standing problem and would require a library paper to do "right." (It would still be easy enough to add the needed algorithms with uglified names, e.g. `__uninitialized_copy_a`, `__destroy_a`, etc. This is exactly what libstdc++ does, and libc++ might be wise to copy its approach.)

I'd be happy to throw together a patch for `__uninitialized_copy_a` etc., since I think that would improve libc++ in general; but I don't see how that would directly help any specific short-term problem in libc++. This patch as it is helps two specific short-term problems:
(1) that user specializations of allocator_traits don't work (but, as the test case comments, this is arguably not a good idea anyway; see also https://quuxplusone.github.io/blog/2018/07/14/traits-classes/ )
(2) that the diff between libc++ trunk and libc++ trivially-relocatable is unnecessarily large
Messing with the uninitialized_foo algorithms would not directly help either of these problems, so we'd have to come up with some other rationale for it.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49317