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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! In general looks good, but I've several minor issue.



================
Comment at: libcxx/include/__bit/byteswap.h:25
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
+  switch (sizeof(_Tp)) {
----------------
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.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:24
+  test_num<uint32_t>(0xABCDABCD, 0xCDABCDAB);
+  test_num<uint64_t>(0xABCDABCDABCDABCD, 0xCDABCDABCDABCDAB);
+
----------------
I would suggest to use '0x0123456789ABCDEF`. That way all nibbles are tested properly.


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


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


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:34
+
+int main(int, char**) {
+  test();
----------------
Since the function requires to be `noexcept` please add an test using `ASSERT_NOEXCEPT` to validate it's really `noexcept`.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:37
+  static_assert(test());
+}
----------------
Needed to make sure it works properly on freestanding implementations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074