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

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

Quuxplusone added a comment.

LGTM % test changes, but I'd like to see the test changes.



================
Comment at: libcxx/include/charconv:115
+operator~(chars_format __x) {
+  return chars_format(~static_cast<__chars_format_underlying_type>(__x) & 0x7);
+}
----------------
Mordante wrote:
> curdeius wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > curdeius wrote:
> > > > > curdeius wrote:
> > > > > > Maybe add a constant for 0x7 to make it clear that it's a bitmask for all possible enumerators.
> > > > > http://eel.is/c++draft/bitmask.types lacks the bitand so that the result is a correct enum value. Would it be an issue in the wording? Or should we stick closely to the standard given that the usage of ~ should be `X &= ~Y`?
> > > > > Maybe add a constant for 0x7 to make it clear that it's a bitmask for all possible enumerators.
> > > > 
> > > > I think that would be useful.
> > > > 
> > > > > http://eel.is/c++draft/bitmask.types lacks the bitand so that the result is a correct enum value. Would it be an issue in the wording? Or should we stick closely to the standard given that the usage of ~ should be `X &= ~Y`?
> > > > 
> > > > Not entirely sure what you mean. Do you mean it's possible to get a value of `0` for the format? That seems possible when using [bitmask.types]/1 and 4.3 implies this is wanted. The caller of `std::to_chars` should ensure this doesn't happen http://eel.is/c++draft/charconv#to.chars-10.
> > > > 
> > > > http://eel.is/c++draft/bitmask.types lacks the bitand so that the result is a correct enum value. Would it be an issue in the wording? Or should we stick closely to the standard given that the usage of ~ should be `X &= ~Y`?
> > > 
> > > 
> > I meant that http://eel.is/c++draft/bitmask.types#2 defines:
> > ```
> > constexpr bitmask operator~(bitmask X){
> >   return static_cast<bitmask>(~static_cast<int_type>(X));
> > }
> > ```
> > whereas you define it as more or less:
> > ```
> > constexpr bitmask operator~(bitmask X){
> >   return static_cast<bitmask>(~static_cast<int_type>(X) & all_bits_of_bitmask);
> > }
> > ```
> > 
> > So, without your change, the result of ~X, where X is a chars_format, would not have a value being one of enumerators of X. Example `~fixed = ~2 = 0xFFFFFFFD` (if 32-bit).
> > 
> > Personally I'd say that it's not important given the intended usage (X&=~Y, http://eel.is/c++draft/bitmask.types#4.2).
> > 
> > On a second thought, I'd remove `& 0x7` altogether. Anyway, WDYT about it?
> > Personally I'd say that it's not important given the intended usage (X&=~Y, http://eel.is/c++draft/bitmask.types#4.2).
> > 
> > On a second thought, I'd remove `& 0x7` altogether. Anyway, WDYT about it?
> 
> That would solve the issue that `0x7` could change in the future. The code I used as template has similar code.
> The bitmasks using `~static_cast<T>(__x)` are in
> * `<filesystem>`
> The bitmask using `~static_cast<T(__x) & V`
> * `<future>`
> * `<regex>`
> I looked at some other bitmask types, but they don't use scoped enums, so don't need their own operators. Of course when using an `unsigned` the value can also be out of the 'valid' bitmasks.
> 
> MSVC STL uses a macro for their bitmask types using `~static_cast<T>(__x)`.
> libstdc++ also uses `~static_cast<T>(__x)`, only looked at `chars_format`.
> 
> So I'm in favour of removing the `& 0x7` and also create a new patch changing `<future>` and `<regex>`.
Pedantically, isn't this sketchy as heck? `~scientific` would be `chars_format(-1)` which I think isn't technically defined behavior? Does the standard actually require this operator to work?
If I'm right (which I may not be), then would giving `chars_format` a proper underlying type help matters? e.g. `enum class chars_format : unsigned int { ... }`
(I wrote out this comment on line 114 before seeing that it duplicated a lot of this comment thread.)


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:69
+         ((~std::chars_format::hex & std::chars_format::general) ==
+          std::chars_format::general);
+}
----------------
Please rewrite this function to use `assert(x); assert(y); assert(z); return true;`
instead of `return x && y && z;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115