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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:24
+  test_num<uint32_t>(0xABCDABCD, 0xCDABCDAB);
+  test_num<uint64_t>(0xABCDABCDABCDABCD, 0xCDABCDABCDABCDAB);
+
----------------
Quuxplusone wrote:
> 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>);
> ```
Regardless whether or not to test the 128-bit failures, I think it would be good to add a test to make sure non-integral types aren't accepted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074