[libcxx-commits] [PATCH] D97115: [libc++] Make chars_format a bitmask type.

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

Quuxplusone added a comment.

LGTM mod nits; but I think the "`operator@=` should be constexpr" nit is a BIG nit.



================
Comment at: libcxx/include/charconv:111
 
+using __chars_format_underlying_type = underlying_type<chars_format>::type;
+
----------------
Mordante wrote:
> curdeius wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > curdeius wrote:
> > > > > Would adding `to_underlying` helper be cleaner?
> > > > > Something along the lines of the proposed http://open-std.org/JTC1/SC22/WG21/docs/papers/2019/p1682r1.html. WDYT?
> > > > I hadn't seen that paper yet, but it seems very useful. According to to https://github.com/cplusplus/papers/issues/460 it seems to be on track to land in C++23. I'm somewhat tempted to implement `__to_underlying` in `utility` for C++11 for now and also adjust the other bitmask types. Then we can switch to `to_underlying` or add a C++17 `to_underlying` once the paper lands. @ldionne What's your opinion?
> > > This comment aged fast; according to Herb P1682 has been adopted [1]. I'll create a patch to implement it. I think it makes sense to allow its usage in C++11 or do we prefer C++23 only? In that case I'll add a private function for C++11 which is used by the C++23 public version.
> > > 
> > > [1] https://herbsutter.com/2021/02/22/trip-report-winter-2021-iso-c-standards-meeting-virtual/
> > @Mordante, I have already an almost ready patch for `to_underlying`. I'll finish it up this evening or tomorrow and send for a review, so don't bother for the tests of your private function.
> Thanks for the information. Then I'll wait for your patch and use that in this patch.
FWIW, you //could// use `constexpr` instead of `_LIBCPP_CONSTEXPR` throughout this C++11-and-later file. I have no opinion on the matter, but I don't know if others might have a preference.


================
Comment at: libcxx/include/charconv:147-151
+inline _LIBCPP_INLINE_VISIBILITY chars_format& operator^=(chars_format& __x,
+                                                          chars_format __y) {
+  __x = __x ^ __y;
+  return __x;
+}
----------------
This should also be `constexpr`, unless I'm mistaken. Even if it's not mandated, certainly it would be //nice QoI// to be able to use `chars_format x, y; x ^= y` in a constexpr function the same way we can use `int a, b; a ^= b` today.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:10
+// Note: chars_format is a C++17 feature backported to C++11. Assert isn't
+// allowed in a constexpr in C++11. To keep the code better readable C++11
+// support is untested.
----------------
...allowed in a constexpr function in C++. To keep the code readable, C++11...


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:29
+
+void test() {
+  {
----------------
See my comment above about constexpr `@=` operators, and if so, make this function also constexpr.


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:58-60
+  assert(static_cast<chars_format_underlying_type>(
+             std::chars_format::hex &
+             (std::chars_format::scientific | std::chars_format::fixed)) == 0);
----------------
Would this test be easier to read as...?
```
assert((std::chars_format::hex & (std::chars_format::scientific | std::chars_format::fixed)) == std::chars_format(0));
```
(or `{}` instead of `(0)`, I have no strong preference)


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:75
+  return true;
+}
+
----------------
It might be worth adding these tests:
```
std::chars_format x;
static_assert(std::is_same<std::chars_format, decltype(~x)>::value, "");
static_assert(std::is_same<std::chars_format, decltype(x&x)>::value, "");
static_assert(std::is_same<std::chars_format, decltype(x|x)>::value, "");
static_assert(std::is_same<std::chars_format, decltype(x^x)>::value, "");
static_assert(std::is_same<std::chars_format&, decltype(x&=x)>::value, "");
static_assert(std::is_same<std::chars_format&, decltype(x|=x)>::value, "");
static_assert(std::is_same<std::chars_format&, decltype(x^=x)>::value, "");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115