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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 6 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/charconv:111
 
+using __chars_format_underlying_type = underlying_type<chars_format>::type;
+
----------------
Quuxplusone wrote:
> 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.
Good point. I prefer `contexpr`.


================
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;
+}
----------------
Quuxplusone wrote:
> 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.
They shouldn't be `constexpr` per http://eel.is/c++draft/bitmask.types#2.
My guess is the bitmask type was added/updated in C++11. IIRC in C++11 a `constexpr` could only have a single `return` and they have never been updated. Guess it would be nice to write a proposal to update this type.

I agree it would be nice to use `constexpr`, but that's not allowed per http://eel.is/c++draft/constexpr.functions.


================
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);
----------------
Quuxplusone wrote:
> 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)
I don't like that, I tried it and our formatting rules still make it not that great. Instead I renamed `chars_format_underlying_type` to `ut` and added `using cf = std::chars_format;`. With these changes all asserts are less than 80 characters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115