[libcxx-commits] [PATCH] D127594: [libc++][NFC] Use concepts in <bit>.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/__bit/bit_cast.h:25-26
+template <class _ToType, class _FromType>
+  requires(sizeof(_ToType) == sizeof(_FromType) && is_trivially_copyable_v<_ToType> &&
+           is_trivially_copyable_v<_FromType>)
+_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _ToType bit_cast(_FromType const& __from) noexcept {
----------------
ldionne wrote:
> 
Fixed, this is clang-format's idea ;-)


================
Comment at: libcxx/include/__bit/bit_cast.h:27
+           is_trivially_copyable_v<_FromType>)
+_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _ToType bit_cast(_FromType const& __from) noexcept {
+  return __builtin_bit_cast(_ToType, __from);
----------------
jloser wrote:
> Nit: can we use west const for consistency in a lot of the other places in `libcxx`? E.g. `s/_FromType const&/const _FromType&`.
Nice catch!


================
Comment at: libcxx/include/bit:94
 {
     static_assert(__libcpp_is_unsigned_integer<_Tp>::value, "__rotl requires an unsigned integer type");
     const unsigned int __dig = numeric_limits<_Tp>::digits;
----------------
jloser wrote:
> Since we're concept constraining the public functions like `std::rotl` and friends, should we just remove these `static_assert`s in the private functions? I don't see these as ever being called from outside this file from the public function.
I prefer to keep them as-is. These functions are there to allow other parts of the code to use these functions before C++20. When they are unused I think it would make more sense to remove these functions and move the implementation into the public files. We've implemented most of C++17 so there shouldn't be much new pre-C++20 code around.

I'll do that in a separate patch.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127594