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

Sat Dec 17 16:08:31 PST 2022

var-const marked 2 inline comments as done.
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
----------------
ldionne wrote:
> Do we have a test with an `_In` and/or an `_Out` that is `volatile`?
Yes (the very last test case in `copy_move_nontrivial.pass.cpp`).

================
Comment at: libcxx/include/__algorithm/copy_move_common.h:51
+    // `memmove` doesn't accept `volatile` pointers, make sure the optimization SFINAEs away in that case.
+    !is_volatile<_In>::value &&
+    !is_volatile<_Out>::value &&
----------------
philnik wrote:
> var-const wrote:
> > philnik wrote:
> > > Does this fix https://llvm.org/PR28901?
> > Hmm, I couldn't immediately see how that issue is related, could you please double-check the link? If the link is correct, then I'm missing some context, sorry.
> Not sure why the LLVM redirect is broken on this, here is the github link I meant: https://github.com/llvm/llvm-project/issues/28901
Yes, it fixes the compilation error when passing pointers to volatile. Thanks for finding the issue!

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:27
+// are representable by `_Up` and vice versa, and built-in assignment between objects of types `_Tp` and `_Up` produces
+// the same value as using `bit_cast` to cast between the types.
+//
----------------
ldionne wrote:
> I would also add a mention of http://eel.is/c++draft/basic.types.general#3 which blesses what we're doing here with trivially copyable types.
Slightly rephrased as
```
In other words, `From` and `To` have the same value representation and the set of values of `From` is a subset of the set of values of `To`.
```
(I think that having the same object representation can be implied, but the bit about having the set of values is important because e.g. a boolean and its corresponding integer type are said to have the same value representation)

I'm not sure about adding that particular section. It explicitly blesses the simple case (copying between objects of the same type), but this trait is primarily for conversions.

================
Comment at: libcxx/include/__type_traits/is_assignment_equivalent_to_bitcast.h:37-80
+  static const bool value = is_trivially_assignable<_To&, _From&>::value &&
+    // First, the simple case -- `From` and `To` are the same object type.
+    ((is_same<_UnqualFrom, _UnqualTo>::value && is_object<_UnqualFrom>::value) ||
+
+    // Beyond the simple case, we say that one type is "bit-cast assignable" to another if:
+    // - (1) `From` and `To` have the same value representation, and in addition every possible value of `From` has
+    //   a corresponding value in the `To` type (in other words, the set of values of `To` is a superset of the set of
----------------
ldionne wrote:
> ```
> // This is the trait for the core type property, separate from syntactic requirements
> template <class _From, class _To> // those are cv-unqualified types always
> struct __is_always_bitcastable {
>   static constexpr bool value = 
>      // basic case, only one type, check that it's trivially copyable
>      (is_same< _From, _To >::value && is_trivially_copyable<_From>::value) ||
>      (
>        sizeof(_From) == sizeof(_To) &&
>        is_integral<_From>::value &&
>        is_integral<_To>::value &&
>        !is_same<_To, bool>::value
>      );
> };
> 
> // Then inside std::copy, do:
> enable_if_t<
>   is_assignable<_From, _To>::value && __is_always_bitcastable<_From, _To>::value
> >
> It copy(...) {
>   // use memcpy, which we know is valid since __is_always_bitcastable is true
> }
> ```
> 
> That should also make the tests for this trait less weird, since now it will be natural that array types are always bitcastable, per the definition of trivially-copyable.
Done. Please check the new rationale for (rejecting) pointers. Arrays are accepted now like we discussed.

================
Comment at: libcxx/test/libcxx/algorithms/alg.modifying.operations/copy_move_trivial.pass.cpp:15-16
+// UNSUPPORTED: modules-build
+// GCC complains about "ambiguating" `__builtin_memmove`.
+// UNSUPPORTED: gcc
+
----------------
ldionne wrote:
> If it's just a warning, you could use `// ADDITIONAL_COMPILE_FLAGS(gcc): -Wno-whatever`.
I'm no longer getting that when running the buildbot locally. Perhaps the warning doesn't cover the case when the ambiguating function is a template. I'll wait for CI to be sure.

================
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>
----------------
ldionne wrote:
> var-const wrote:
> > 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).
> I would argue that's a "libc++ policy" decision and it's contrary to what we had agreed on when we started granularizing headers. IIRC, we had decided that a header like `<foo>` should include everything inside `<__foo/*.h>`, regardless of whether it's public API or not. That being said, that simply doesn't work -- I've run into cases where including everything inside `<__foo/*.h>` leads to circular dependencies, so this is something we'll likely have to revisit. The status quo is also that we are not systematically including all of `<__foo/*.h>`. So I'm fine with what you've done.
Thanks for explaining the additional context!

================
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]>>();
+  }
----------------
ldionne wrote:
> var-const wrote:
> > I found it surprising, but arrays are currently considered bit-convertible. What do you think?
> I think that for the purpose of this trait, we should probably also enforce that the two types are `std::is_trivially_assignable_v<To, From>`, and that would handle the array case. WDYT?
> 
> Perhaps this means that the name and purpose of the trait must change a tiny bit, maybe something like `__copy_assignment_is_bitcast`? Basically anything that will hint at the fact that this does a bit more than checking whether the two types are bit-castable, since we're also taking into account other unrelated language rules like `t = u` being valid. We should make sure to have a test that would catch it if our `std::copy` suddenly started to accept things that are not assignable to each other just because we are using `std::memcpy` under the hood.
I went with `__is_assignment_equivalent_to_bitcast` which is quite a mouthful -- happy to improve the name.

We might split this into two traits (one that doesn't check for `is_trivially_assignable` but checks everything else, and the other is a conjunction of the first trait and `is_trivially_assignable`), but I'd wait until we have a use case for that (assuming it's a good idea in the first place).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139235