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

Thu Dec 8 12:09:52 PST 2022

ldionne accepted this revision.
ldionne added a subscriber: jfb.
ldionne added a comment.
This revision is now accepted and ready to land.

I'll have another look but this LGTM w/ comments applied. Thanks a lot for all the work on this patch, the rabbit hole is much deeper than I initially thought.

================
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
----------------
Do we have a test with an `_In` and/or an `_Out` that is `volatile`?

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
@jfb Just tagging you here in case you're curious to take a look. Your pair of eyes on this file would certainly be a bonus.

================
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.
+//
----------------
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.

================
Comment at: libcxx/include/__type_traits/are_always_bitcastable.h:59-61
+    // - booleans normally use only a single bit of their possible value range; bit-casting an integer to a boolean will
+    //   result in a boolean object with an incorrect representation, and such an object may compare both `true` and
+    //   `false` in different contexts (fails #3);
----------------

================
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:
> var-const wrote:
> > Do we still need this in new code?
> Do we need to inherit from `integral_constant` for internal traits?
`_LIBCPP_TEMPLATE_VIS` isn't needed, not because it's new code, but because this gives default visibility to a struct that doesn't require any visibility. There's nothing to give visibility to.

> Do we need to inherit from `integral_constant` for internal traits?

No.

================
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).
----------------
var-const wrote:
> 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.
I think I like this.

================
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
+
----------------
If it's just a warning, you could use `// ADDITIONAL_COMPILE_FLAGS(gcc): -Wno-whatever`.

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:62-65
+template <class Types>
+constexpr void check() {
+  check_with_expected<true, Types, Types>();
+}
----------------
Then you get the same benefit for eliminating repetition, but IMO this is less arbitrary. And for the "same types" case, we're just instantiating the same function twice (well once really because compiler memoization).

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:67-70
+template <class T>
+constexpr void check_simple() {
+  check<meta::type_list<T>>();
+}
----------------
IMO this doesn't pull its weight.

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

================
Comment at: libcxx/test/libcxx/type_traits/are_always_bitcastable.compile.pass.cpp:205
+  {
+    check_false<meta::type_list<int&>, meta::type_list<int&>>();
+  }
----------------
var-const wrote:
> This is the behavior that we want, right?
I think so, because references are not objects.

================
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]>>();
+  }
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139235