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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/charconv:115
+operator~(chars_format __x) {
+  return chars_format(~static_cast<__chars_format_underlying_type>(__x) & 0x7);
+}
----------------
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>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115