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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
