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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM, just some nitpicky irrelevancies. :)  If you're happy with the (lack of) constexpr tests, I say ship it already.



================
Comment at: libcxx/docs/UsingLibcxx.rst:305
 * ``binary_search``
+* ``bit_cast``
 * ``clamp``
----------------
I put this down between `as_const` and `forward`, which is a-z among the whatever-paper-that-was casting operators, instead of up here with the STL algorithms. `bit_cast` is not an STL algorithm.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp:38
+        assert(std::memcmp(&to, &middle, sizeof(T)) == 0);
+        assert(std::memcmp(&from, &to, sizeof(T)) == 0);
+        assert(std::memcmp(&middle, &middle2, sizeof(Buffer)) == 0);
----------------
I'd checked `from,middle` and `middle,to`, in which case transitivity gets us `from,to`.
(This is just straight comparison of arrays of bytes, so no evil type can trick us here; transitivity definitely applies.)


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp:39
+        assert(std::memcmp(&from, &to, sizeof(T)) == 0);
+        assert(std::memcmp(&middle, &middle2, sizeof(Buffer)) == 0);
+    }
----------------
By definition, `sizeof(Buffer)` equals `sizeof(T)`, or else `bit_cast` would not compile... right?
I preferred to consistently use `T` throughout (and below instead of `Nested`), but I guess it doesn't matter.


================
Comment at: libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp:253
+int main(int, char**) {
+    tests();
+    static_assert(basic_constexpr_test());
----------------
I guess I meant to try to constexpr-ify `tests()` but never succeeded. The problems were (I think) with `std::nanl` and `std::memcmp`. I think it would be fine to ship this with only the `basic_constexpr_test`; although you might also try putting the `memcmp`s under `if (!std::is_constant_evaluated())` and splitting out the floating-point `nan` tests into a non-constexpr special test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75960