[libcxx-commits] [PATCH] D75960: [libc++] Implement C++20's P0476r2: std::bit_cast

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

LGTM % comments, FWIW.



================
Comment at: libcxx/include/CMakeLists.txt:96-98
   __bit_reference
+  __bit/bit_cast.h
   __bits
----------------
Nit: `/` < `_` < `a` according to ASCII.
But note that we get this wrong for `__functional_base` vs. `__functional/...` below. So if you take this nit, maybe fix both places; and/or maybe wonder are we historically sorting by something //other// than ASCIIbetically?


================
Comment at: libcxx/include/__bit/bit_cast.h:29
+>>
+_LIBCPP_HIDE_FROM_ABI
+constexpr _ToType bit_cast(_FromType const& __from) noexcept {
----------------
Please `_LIBCPP_NODISCARD_EXT`, and uncomment the existing test in `libcxx/test/libcxx/diagnostics/nodiscard_extensions.pass.cpp` which was added by D99895.

Please also add `bit_cast` to the list in `libcxx/docs/UsingLibcxx.rst`.

Please also `git grep bit_cast libcxx/` and see if there are any more things waiting to be uncommented by this patch.


================
Comment at: libcxx/include/__bit/bit_cast.h:31
+constexpr _ToType bit_cast(_FromType const& __from) noexcept {
+    return __builtin_bit_cast(_ToType, __from);
+}
----------------
Surely this should be guarded by `__has_builtin(__builtin_bit_cast)`. Or is the rationale that we don't need to guard it, because we have no fallback implementation, and so "you get a weird compiler error" is already the best-case outcome on compilers that don't support it?  Notice that because `_ToType` is a //type//, not a //value//, you'll get a parse error very eagerly as soon as `<bit>` is included; it's not merely that you'll get an error when you try to //call// `bit_cast`.

It looks like the builtin is supported on Clang 9+ and GCC 11+; it's unsupported on Clang 8- and GCC 10-.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.compile.pass.cpp:36
+    struct From { char a; char b; };
+    static_assert(!bit_cast_is_valid<To, From>);
+}
----------------
Consider adding things like
```
static_assert(!bit_cast_is_valid<char[2], From>);  // makes bit_cast's return type invalid
static_assert(!bit_cast_is_valid<int(), From>);  // duh, but might as well check
static_assert(bit_cast_is_valid<From, From>);  // OK, right?
static_assert(!bit_cast_is_valid<From&, From>);  // Not OK???
```
I'm not sure about the last one. Obviously real production code should use `reinterpret_cast`, not `bit_cast`, for such an operation; but I don't know if `bit_cast` is supposed to accept it or not.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.compile.pass.cpp:49
+    struct To { char a; };
+    struct From { char a; From() { } From(From const& x) : a(x.a) { } };
+    static_assert(!bit_cast_is_valid<To, From>);
----------------
`From() { }` is superfluous, right? Remove.
Consider removing the function bodies, too.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp:94
+
+    // Fundamental signed integer types
+    for (short i : generate_signed_integral_values<short>()) {
----------------
Here you're missing `signed char`, and below you're missing `unsigned char`. Also all the `charN_t` types, not that I'm asking for those.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp:109
+        test_roundtrip_through_buffer(i);
+        test_roundtrip_through<double>(i);
+    }
----------------
I'm sure there must be supported platforms where `long` and `double` are different sizes. (If Linux isn't one, then Windows is probably one; and vice versa.)
...ah, I see, this line simply belongs on line 114 (under `long long`) instead. Ditto line 132 should be moved to 137.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75960