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

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

Quuxplusone added inline comments.


================
Comment at: include/vector:318
+    }
+}
+
----------------
mclow.lists wrote:
> Quuxplusone wrote:
> > Marshall writes:
> > > Instead, we should be writing simple loops that the compiler can optimize into a call to memcpy if it chooses. Having calls to memcpy in the code paths makes it impossible to "constexp-ify" this code.
> > 
> > Well, I have three thoughts on that.
> > 
> > (A), "removing the calls to memcpy" sounds like you want to just call the *actual* move-constructor in a loop, and then later call the actual destructor in a loop. Which is to say, you don't want libc++ to have a codepath for this speed optimization at all. That's just leaving a ton of performance on the table, and I strongly disagree with that idea.
> > 
> > (B), regardless, couldn't you achieve that goal simply by taking this patch almost exactly as it is except removing the overloads that take `true_type`? If you want constexpr-friendliness badly enough that you're willing to call the move-constructor and destructor even of trivially copyable types, then you can still use this framework; you just have to remove the overloads that call memcpy. That wouldn't be a major refactoring.
> > 
> > (C), surely if you want the best of both worlds, you should be pushing someone to invent a constexpr memcpy and/or a way to [detect constexpr-context at compile time](https://quuxplusone.github.io/blog/2018/06/12/perennial-impossibilities/#detect-the-constexprness-of-the-current-context)? I don't think it makes sense to pessimize existing (non-constexpr) users in C++03-through-C++17 just because someone hypothetically might in C++2a-or-later want to mutate a std::vector in a constexpr context.
> > Which is to say, you don't want libc++ to have a codepath for this speed optimization at all. 
> 
> You're completely correct. I don't want libc++ to have such a code path.
> I want clang to generate a `memcpy` from the code w/o ever mentioning `memcpy` in the source.
> 
> 
@mclow.lists: So would you accept a version of this patch that simply removed the `true_type` overloads? That would change this from a pure refactoring to a performance regression, but it would still reduce the overall diff between libc++ master and libc++ trivially-relocatable.

(Maybe it's no longer clear and needs restating: This patch is //currently// a pure refactoring. All I'm doing is moving the //existing// helper functions out of allocator_traits. IIUC, your objections apply to the existence of these //existing// helper functions just as much as to the refactored versions.)


Repository:
  rCXX libc++

https://reviews.llvm.org/D49317