[libcxx-commits] [PATCH] D61761: P1144 "Trivially relocatable" (1/3): is_trivially_relocatable, relocate_at, and uninitialized_relocate

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: include/memory:3466
+    __source->~_Tp();
+    return _VSTD::__launder(__dest);
+}
----------------
Quuxplusone wrote:
> I'm not sure whether this `launder` is doing anything.
> Alternatively, should I assign `__dest = ::new (...` and then `return __dest`?
That seems more correct. After calling `new` the pointer should still be active, so you won't need `launder `.

I think `launder` is only necessary after `memmove` because then the pointer's lifetime is over.


================
Comment at: include/memory:3510
+    for (; __first != __last; (void)++__result, ++__first) {
+        ::new (static_cast<void*>(_VSTD::addressof(*__result))) _Dt(_VSTD::move(*__first));
+        _VSTD::destroy_at(std::addressof(*__first));
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > I think this should require `_LIBCPP_STD_VER > 03`.
> libc++ supports `std::move` even in `_LIBCPP_HAS_NO_RVALUE_REFERENCES`-mode, presumably because it would be too tedious to `#ifdef` all the places where we use `std::move` (such as here).
Good to know.


================
Comment at: include/memory:3556
+        integral_constant<bool,
+            !is_volatile<_St>::value && !is_volatile<_Dt>::value &&
+            __is_same_uncvref<_St, _Dt>::value &&
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > What is the reason for `is_volatile`? I don't see that in the paper (but maybe I am missing something).
> It's related to https://quuxplusone.github.io/blog/2018/07/13/trivially-copyable-corner-cases/ — if we have a range of `volatile Foo`, then the user is probably expecting us to follow the abstract machine and copy the `Foo` objects one by one, instead of byte-by-byte (possible tearing). The library doesn't technically //have// to follow the abstract machine here, but I think it's QoI to do so.
> 
> I just re-examined `std::copy` and it doesn't bother to check `is_volatile`, so I guess it would be consistent for libc++ not to check `is_volatile` here either. (That would make the QoI //consistently// low, IMHO.)
Maybe open an issue about this (in bugzilla) so that once/if P1153 is introduced it can be updated.


================
Comment at: include/memory:3464
+_Tp *__relocate_at_impl(_Tp *__source, _Tp *__dest, false_type) {
+    ::new (static_cast<void*>(__dest)) _Tp(_VSTD::move(*__dest));
+    __source->~_Tp();
----------------
Are you intentionally not doing anything with the result of `new`? Also, I am not sure you need to `static_cast` to `void*`.

Also, also, I think you want to move `__source` not `__dest` (because I am pretty sure `__dest` is where you want to relocate the new thing and `__source` is where you want to get it).


================
Comment at: include/memory:3481
+        __source, __dest,
+        typename is_trivially_relocatable<_Tp>::type()
+    );
----------------
I think this should be `is_trivially_relocatable<_Tp>()`.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61761/new/

https://reviews.llvm.org/D61761