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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM mod comments. One (int->unsigned int) is functional; the others are just style improvements.



================
Comment at: libcxx/include/__bit/byteswap.h:49
+#endif // _LIBCPP_HAS_NO_INT128
+}
+
----------------
Style nit: Personally, I'd add `{ }` around each `if`'s body, and around the `else`, and cuddle the `else`. This gives the same number of source lines, but makes it much easier to add or remove lines without major surgery.
```
if constexpr (X) {
} else if constexpr (Y) {
} else {
}
```
(That said, libc++ style //is// generally to omit braces on single-line `if`s... but that's usually because it saves a line to omit the braces. Here, it doesn't save you anything, and you've got a mix of bodies-that-need-braces and bodies-that-don't, so "consistency" comes into play too.)
Anyway, not a blocker AFAIC.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:95-102
+  test_num<short>(0x1358, 0x5813);
+  test_num<unsigned short>(0x1358, 0x5813);
+  test_num<int>(0x45612378, 0x78236145);
+  test_num<int>(0x45612378, 0x78236145);
+  test_implementation_defined_size<long>();
+  test_implementation_defined_size<unsigned long>();
+  test_implementation_defined_size<long long>();
----------------
Seems like it would be simpler to use the same approach for //all// of these types, wouldn't it?
And `get_test_data` and `get_expected` could be moved into a single function `std::pair<T, T> get_test_data()` so that you wouldn't have to keep lines in sync that were separated by ~15 LOC.


================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:98
+  test_num<int>(0x45612378, 0x78236145);
+  test_num<int>(0x45612378, 0x78236145);
+  test_implementation_defined_size<long>();
----------------
`s/int/unsigned int/` IIUC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074