[libcxx-commits] [PATCH] D114074: [libc++] Implement P1272R4 (std::byteswap)

[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 (most of which echo @Mordante's comments) — thanks! Requesting changes so that I'll see it again.



================
Comment at: libcxx/include/__bit/byteswap.h:25
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
+  switch (sizeof(_Tp)) {
----------------
Mordante wrote:
> We conditionally support `__uint128_t`. This type won't work with the current implementation. I would prefer to support that type, or constrain function where `sizeof(_Tp) <= 8`. The availability is guarded by `_LIBCPP_HAS_NO_INT128`.
> 
> Note that GCC already has a `__builtin_bswap128`, so maybe it's interesting to also add this to clang. I wouldn't mind looking into that myself if you don't want to.
IMO:
- Definitely add `_LIBCPP_UNREACHABLE();` at the end of this function.
- If both Clang and GCC support `__builtin_bswap128` then we should just do that for `case 16`.
- Otherwise, we should `case 16: return _Tp(byteswap(uint64_t(__val))) | (_Tp(byteswap(uint64_t(__val >> 64))) << 64); // TODO: __builtin_bswap128 when Clang supports it`.


================
Comment at: libcxx/include/__bit/byteswap.h:38
+
+#endif
+
----------------
Since you've used `std::integral` on line 24, you need to guard with `#if _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_CONCEPTS)`. Also, please mark the `#endif`:
```
#endif // _LIBCPP_STD_VER > 20 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
```


================
Comment at: libcxx/include/__bit/byteswap.h:42
+
+#endif
----------------



================
Comment at: libcxx/include/bit:58
     native = see below         // C++20
 };
 
----------------
This line is misindented.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:17
+constexpr void test_num(T in, T expected) {
+  assert(std::byteswap(in) == expected);
+}
----------------
Please also
```
ASSERT_SAME_TYPE(decltype(std::byteswap(in)), decltype(in));
ASSERT_NOEXCEPT(std::byteswap(in));
```


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:24
+  test_num<uint32_t>(0xABCDABCD, 0xCDABCDAB);
+  test_num<uint64_t>(0xABCDABCDABCDABCD, 0xCDABCDABCDABCDAB);
+
----------------
Mordante wrote:
> Mordante wrote:
> > Mordante wrote:
> > > I would suggest to use '0x0123456789ABCDEF`. That way all nibbles are tested properly.
> > We need to either test `__int128_t` / `__uint128_t` here if you want to support it. If you don't want to support it we need a separate validate test that rejects this type.
> I really would like see tests for all integral types. https://en.cppreference.com/w/cpp/types/is_integral, for example `bool` and the character types aren't tested now.
+1 for testing all integral types, including `char` and `bool`.
Also, please add a simple neg test. I recommend:
```
template<class T>
concept has_byteswap = requires (T t) { std::byteswap(t); };

static_assert(!has_byteswap<void*>);
static_assert(!has_byteswap<float>);
static_assert(!has_byteswap<char[2]>);
static_assert(!has_byteswap<std::byte>);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074