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

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

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: include/vector:296
+_LIBCPP_INLINE_VISIBILITY
+inline void
+__copy_construct_forward(_Alloc& __a, _Iter __begin1, _Iter __end1,
----------------
ldionne wrote:
> Do you really need `inline` here?
I'm actually not sure — and also suddenly not sure if the visibility attribute should be `_LIBCPP_INLINE_VISIBILITY` or `_LIBCPP_TEMPLATE_VIS`. (I *think* the latter is only for type templates, though, not function templates?)  However, this is exactly parallel to what we do for `operator<`, so I think changing it would be gratuitous. If someone wants to remove `inline` from a bunch of templates, I won't complain, but I also don't want this PR to be the one that initiates it.

```
template <class _Tp, class _Allocator>
inline _LIBCPP_INLINE_VISIBILITY
bool
operator< (const vector<_Tp, _Allocator>& __x, const vector<_Tp, _Allocator>& __y)
{
    return _VSTD::lexicographical_compare(__x.begin(), __x.end(), __y.begin(), __y.end());
}
```



================
Comment at: include/vector:545
+    is_trivially_move_constructible<_Tp>::value
+> {};
+
----------------
Louis writes:
> It would be nice if all the TMP required to determine whether to call `__move_construct_forward(..., true_type)` or `__move_construct_forward(..., false_type)` was done in `__move_construct_forward` itself (or a helper). This way, callers wouldn't have to do it themselves.

I know where you're coming from, but I believe that in this case we definitely *can't* do that, because the whole point of these routines is that the routine itself can't always tell whether it's supposed to memcpy or not; the *caller* is the only one with the power to decide that. The decision (in theory, though not yet in practice, because this particular PR is a pure refactoring) depends not only on details of `_Tp` and `_Allocator` but also on the specific call-site: we can memcpy more aggressively at some call-sites than others, because of information available only to the caller (such as "this is a relocation operation").

See https://github.com/Quuxplusone/libcxx/commit/e7e5999b01#diff-07c2b769648850d040dcbb07754e5f2fR1076 , lines 1076 et seq., for how I envision some future caller making the decisions on a callsite-by-callsite basis.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49317