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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:37
+          class _Out = __libcpp_remove_reference_t<_OutRef>>
+using __enable_if_memmovable = __enable_if_t<
+    sizeof(_In) == sizeof(_Out) &&
----------------
I would make this a proper boolean trait instead. It may make it easier to reuse elsewhere outside of SFINAE contexts.


================
Comment at: libcxx/include/__algorithm/copy_move_common.h:38-44
+    sizeof(_In) == sizeof(_Out) &&
+    is_trivially_assignable<_OutRef, _InRef>::value &&
+    (std::is_same<_In, _Out>::value || (
+                                        std::is_integral<_In>::value &&
+                                        std::is_integral<_Out>::value &&
+                                        !std::is_same<_In, bool>::value &&
+                                        !std::is_same<_Out, bool>::value))
----------------
After our discussion about `is_trivially_copyable` and `is_trivially_<operation>`, I think I would change this to:

```
(is_same<_In, _Out>::value && is_trivially_copyable<_In>::value) ||
(
  sizeof(_In) == sizeof(_Out) &&
  is_integral<_In>::value && is_integral<_Out>::value &&
  !(is_same<remove-cv(_In), bool>::value || is_same<remove-cv(_Out), bool>::value)
)
```

I read this as "first, the obviously correct case", and then "special casing for integral types that we know have the same value representation".

And then we could rename this to `__type_traits/is_always_bitcastable`? It's ugly but I think it conveys the idea of what we're trying to do here, which is essentially tell types for which `bit_cast<To>(From)` is *always* valid and never UB. This trait should also be documented (via comment).


================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_nontrivial.pass.cpp:126
+
+    From input[N] = {From(1), From(2), From(3), From(4)};
+    To output[N];
----------------
Why not this? That will make the testing below more complicated for `bool` but I think it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139235