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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Some small nits, but I'd like to see the CI green before accepting.



================
Comment at: libcxx/include/__bit/byteswap.h:25
+
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
----------------
Quuxplusone wrote:
> Mordante wrote:
> > I still think it would be good to constrain the template on the maximum supported `sizeof(_Tp)`. I prefer a compile-time error over run-time bug or error.
> Constraints are evil because they cause it to just quietly drop out of overload resolution. I don't think WG21 knows what it's doing these days by adding constraints on everything. At first it seems neato that `using std::byteswap; byteswap(N::Widget())` will quietly choose ADL `N::byteswap` instead of being ambiguous — but really, are we //encouraging// people to call `byteswap` by ADL? I don't think so.
> I think it'd be fine to add a static_assert, though, like
> ```
> static_assert(sizeof(_Tp) == 1 || sizeof(_Tp) == 2 || sizeof(_Tp) == 4 || sizeof(_Tp) == 8 || sizeof(_Tp) == 16);
> ```
> Could even refactor this function to use a ladder of `if constexpr ... else if constexpr ...` and then put `static_assert(sizeof(_Tp) == 0, "byteswap is unimplemented for integral types of this size");` in the final `else` branch.
Fair point, I've no objection to `static_assert`s either, just as long as we make sure we don't accept invalid code.


================
Comment at: libcxx/include/__bit/byteswap.h:36
+    return __builtin_bswap64(__val);
+#  ifndef _LIBCPP_HAS_NO_INT128
+  case 16:
----------------
Quuxplusone wrote:
> Nit: This indentation is bizarre. I expect clang-format is to blame.  `#ifndef` with no space, plz. :)
@Quuxplusone That clang-format rule changed in D109835.



================
Comment at: libcxx/include/__bit/byteswap.h:28
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
+  static_assert(sizeof(_Tp) == 1 || sizeof(_Tp) == 2 || sizeof(_Tp) == 4 || sizeof(_Tp) == 8 || sizeof(_Tp) == 16);
+
----------------
Or something similar to make sure we don't except 128 bit values when not supported.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:61
+template <class T>
+constexpr void test_unknown_size() {
+  test_num<T>(get_test_data<T>(), get_expected<T>());
----------------
I think this name is misleading. I would suggest a name like `test_implementation_defined_size`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074