[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:111
 
+using __chars_format_underlying_type = underlying_type<chars_format>::type;
+
----------------
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?


================
Comment at: libcxx/include/charconv:115
+operator~(chars_format __x) {
+  return chars_format(~static_cast<__chars_format_underlying_type>(__x) & 0x7);
+}
----------------
Maybe add a constant for 0x7 to make it clear that it's a bitmask for all possible enumerators.


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


================
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>(
----------------
Given that the exact values of enumerators are unspecified, this test is technically libc++-specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115