ldionne accepted this revision. ldionne added a comment. This revision is now accepted and ready to land.
LGTM ================ Comment at: include/vector:296 +_LIBCPP_INLINE_VISIBILITY +inline void +__copy_construct_forward(_Alloc& __a, _Iter __begin1, _Iter __end1, ---------------- Quuxplusone wrote: > 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()); > } > ``` > Sure. Then, the current one is correct. You want to be using `_LIBCPP_INLINE_VISIBILITY` here. Actually, you want to be using `_LIBCPP_HIDE_FROM_ABI`, but don't start doing this in this commit -- I'll do a bulk replacement later. ================ Comment at: include/vector:545 + is_trivially_move_constructible<_Tp>::value +> {}; + ---------------- Quuxplusone wrote: > 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. Got it. Repository: rCXX libc++ https://reviews.llvm.org/D49317 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits