[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:147-151
+inline _LIBCPP_INLINE_VISIBILITY chars_format& operator^=(chars_format& __x,
+                                                          chars_format __y) {
+  __x = __x ^ __y;
+  return __x;
+}
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > This should also be `constexpr`, unless I'm mistaken. Even if it's not mandated, certainly it would be //nice QoI// to be able to use `chars_format x, y; x ^= y` in a constexpr function the same way we can use `int a, b; a ^= b` today.
> > They shouldn't be `constexpr` per http://eel.is/c++draft/bitmask.types#2.
> > My guess is the bitmask type was added/updated in C++11. IIRC in C++11 a `constexpr` could only have a single `return` and they have never been updated. Guess it would be nice to write a proposal to update this type.
> > 
> > I agree it would be nice to use `constexpr`, but that's not allowed per http://eel.is/c++draft/constexpr.functions.
> The code you quoted is just "exposition-only"; I think the normative text in http://eel.is/c++draft/bitmask.types#1 takes precedence, and that normative text clearly states that integer types //are// bitmask types. Integer types have constexpr assignment ops. Thus, bitmask types //can// have constexpr assignment ops, even if the exposition-only code does depict a specific example type without constexpr support.
> 
> Right now, libc++ is the only one of the Big Three to provide compound operators that don't work in constexpr functions. Let's fix that bug and help users write more portable code. https://godbolt.org/z/r7fzo4oKh
> The code you quoted is just "exposition-only"; I think the normative text in http://eel.is/c++draft/bitmask.types#1 takes precedence, and that normative text clearly states that integer types //are// bitmask types. Integer types have constexpr assignment ops. Thus, bitmask types //can// have constexpr assignment ops, even if the exposition-only code does depict a specific example type without constexpr support.

Good point.

> Right now, libc++ is the only one of the Big Three to provide compound operators that don't work in constexpr functions. Let's fix that bug and help users write more portable code. https://godbolt.org/z/r7fzo4oKh

Indeed, it would be good for the users if the big 3 behave in the same manner. Thanks for this additional information.

I'll adjust the code and update the unit tests. (Note since we allow this code in C++11, I'll only make it `constexpr` in C++14 and newer.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97115