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

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


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


================
Comment at: libcxx/test/std/utilities/charconv/charconv.syn/chars_format.pass.cpp:48
+  return (static_cast<std::__chars_format_underlying_type>(
+              std::chars_format::scientific) == 1) &&
+         (static_cast<std::__chars_format_underlying_type>(
----------------
Mordante wrote:
> curdeius wrote:
> > Given that the exact values of enumerators are unspecified, this test is technically libc++-specific.
> IMO opinion http://eel.is/c++draft/bitmask.types  strongly implies we need to use these values. However I don't mind to make the tests not depending on the exact value.
I'm not a standardese expert but I read values in http://eel.is/c++draft/bitmask.types#2 as an example (exposition only), and that's http://eel.is/c++draft/bitmask.types#3 that defines these values, which should be nonzero, distinct and have no common set bits. So, theoretically, the values could be e.g.  3, 4, 24.
But anyway, that would be silly.
I'm ok if you keep it as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115