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

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

Quuxplusone marked 5 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: include/memory:3463
+inline _LIBCPP_INLINE_VISIBILITY
+_Tp *__relocate_at_impl(_Tp *__source, _Tp *__dest, _IsMemcpyable) {
+    ::new (static_cast<void*>(__dest)) _Tp(_VSTD::move(*__dest));
----------------
zoecarver wrote:
> Nit: is `_IsMemcpyable`  guaranteed to be `true_type` or `false_type`? If so, maybe make it specifically `false_type`.
Brain fart on my part. (This isn't a class template with a partial specialization, it's two overloaded function templates!) Will fix.


================
Comment at: include/memory:3466
+    __source->~_Tp();
+    return _VSTD::__launder(__dest);
+}
----------------
I'm not sure whether this `launder` is doing anything.
Alternatively, should I assign `__dest = ::new (...` and then `return __dest`?


================
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));
----------------
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).


================
Comment at: include/memory:3556
+        integral_constant<bool,
+            !is_volatile<_St>::value && !is_volatile<_Dt>::value &&
+            __is_same_uncvref<_St, _Dt>::value &&
----------------
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.)


================
Comment at: test/std/utilities/meta/meta.unary/meta.unary.prop/is_trivially_relocatable.pass.cpp:14
+
+// UNSUPPORTED: c++03
+// XFAIL: gcc-4.9
----------------
zoecarver wrote:
> Shouldn't this also contain `c++98`?
Hmm. My impression had been that `c++98` and `c++03` were synonymous, and that the tests only ever passed `c++03`. But now I see that most tests //do// refer to `c++98, c++03`, except for the ones I added and also

test/std/diagnostics/syserr/is_error_code_enum.pass.cpp
test/std/input.output/iostreams.base/is_error_code_enum_io_errc.pass.cpp
test/std/utilities/time/time.duration/time.duration.alg/abs.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/ceil.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/floor.fail.cpp
test/std/utilities/time/time.duration/time.duration.cast/round.fail.cpp
test/std/utilities/time/time.point/time.point.cast/ceil.fail.cpp
test/std/utilities/time/time.point/time.point.cast/floor.fail.cpp
test/std/utilities/time/time.point/time.point.cast/round.fail.cpp

Should these other tests also get `c++98` added, or should `c++98` and `c++03` be synonymous in tests?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61761