[libcxx-commits] [PATCH] D139235: Revert "Revert "[libc++][ranges]Refactor `copy{, _backward}` and `move{, _backward}`""

Wed Dec 7 01:27:14 PST 2022

var-const added inline comments.

================
Comment at: libcxx/include/__algorithm/copy_move_common.h:41
+    // `memmove` doesn't accept `volatile` pointers, make sure the optimization SFINAEs away in that case.
+    !is_volatile<_In>::value &&
+    !is_volatile<_Out>::value
----------------
I think the `volatile` condition is only relevant for `memmove` and friends -- `bit_cast` is happy to convert between volatile and non-volatile, and `__are_always_bitcastable` should probably follow suit.

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:25
+
+// Checks whether `_Tp` and `_Up` are always bit-castable to one another. This means that all possible values of `_Tp`
+// are representable by `_Up` and vice versa, and built-in assignment between objects of types `_Tp` and `_Up` produces
----------------
This is my attempt to formalize the trait. Some things that I think are worth calling out:

- the way this is formulated is symmetrical -- not only `T` has to be bit-castable to `U`, but also `U` to `T`. Without this requirement, I wasn't sure how to express that the object size needs to be the same (if `U` is larger, then `U` can represent all values of `T`, so having this as just a one-way relationship didn't seem to work);

- related to the previous point, there is no precedent (that I found) for a type trait with a "plural" name (i.e., prefixed with `are` and not `is`). I chose the plural prefix to stress that this is a symmetric relationship between types;

- I chose to discard cv-qualifiers (IIUC, they don't affect `bit_cast`). `memmove` and friends don't work with `volatile` types, so this trait isn't fully sufficient to check whether `memmove` can be called;

- please double-check my reasoning. Part of the reason I tried to go through every possible case is to make it easier to notice if any of my assumptions are wrong.

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:33
+          class _Up = __remove_cv_t<_MaybeQualifiedU> >
+struct _LIBCPP_TEMPLATE_VIS __are_always_bitcastable : public integral_constant<bool,
+    // First, the simple case -- `T` and `U` are the same type that is trivially copyable.
----------------
Do we still need this in new code?

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:33
+          class _Up = __remove_cv_t<_MaybeQualifiedU> >
+struct _LIBCPP_TEMPLATE_VIS __are_always_bitcastable : public integral_constant<bool,
+    // First, the simple case -- `T` and `U` are the same type that is trivially copyable.
----------------
var-const wrote:
> Do we still need this in new code?
Do we need to inherit from `integral_constant` for internal traits?

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:45-47
+    // To summarize, two types are "always bit-castable" if `u = t` is a valid trivial expression that produces the same
+    // value as `bit_cast<U>(t)` for any possible value of `t`, and conversely for `t = u` (assuming `t` and `u` are
+    // objects of types `T` and `U`, respectively).
----------------
This is the closest I could get to a "formal" definition of what this trait is trying to do. Please let me know what you think.

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:67
+    //   (fails #2);
+    // - arrays can't be assigned to each other using built-in assignment (fails #2);
+    // - there is no need to consider `nullptr_t` for practical purposes.
----------------
>From testing, this currently isn't true -- somehow e.g. `int[1]` is considered bit-castable to itself.

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:68
+    // - arrays can't be assigned to each other using built-in assignment (fails #2);
+    // - there is no need to consider `nullptr_t` for practical purposes.
+    (
----------------
(or so I think)

================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:237
+
+void test_different_signedness() {
+  auto check = [](auto alg) {
----------------
Please let me know if you think this test is overkill (or alternatively, if you'd like to test more types -- I'm operating on the assumption that if it works for one signed/unsigned pair, it works for all of them).

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:13
+//
+// __are_always_bitcastable<_Tp, _Up>
+
----------------
Please let me know if you think these tests are overkill.

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:16
+#include "test_macros.h"
+TEST_CLANG_DIAGNOSTIC_IGNORED("-Wprivate-header")
+#include <__type_traits/are_always_bitcastable.h>
----------------
I think it's better to avoid including this file from `<type_traits>` -- users shouldn't pay the extra compilation penalty for our internal stuff (even if that penalty is very small).

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:205
+  {
+    check_false<meta::type_list<int&>, meta::type_list<int&>>();
+  }
----------------
This is the behavior that we want, right?

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:211
+  {
+    check_false<meta::type_list<int[1]>, meta::type_list<int[1]>>();
+  }
----------------
I found it surprising, but arrays are currently considered bit-convertible. What do you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139235