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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante planned changes to this revision.
Mordante added a comment.

Thanks for your feedback! I'll wait for @ldionne's view on `to_underlying` before addressing the comments.



================
Comment at: libcxx/include/charconv:111
 
+using __chars_format_underlying_type = underlying_type<chars_format>::type;
+
----------------
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?


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



================
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:
> > 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`?




================
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>(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115