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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
Comment at: libcxx/include/__bit/byteswap.h:25
+
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
----------------
Mordante wrote:
> 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.
I'd also prefer the `if constexpr` chain with a `static_assert` in the false case as Arthur mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074