[libcxx-commits] [PATCH] D117736: [libc++][P2321R2] Add vector<bool>::reference::operator=(bool) const

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

(1) The code itself looks great. I'd like to see a test case for this: both testing `is_assignable_v`, and testing its runtime behavior.
(2) You're missing the `constexpr`. But I know that's because we don't support constexpr vector yet. But but, there's no reason we couldn't constexpr `__bit_reference` itself right now today! Any appetite to do that soon, in a separate PR?
(3) Just for curious readers... I asked on Slack: Why is C++23 adding //only one// const overload of `operator=`, when there are //two// signatures already? Shouldn't there be a `const __bit_reference& operator=(const __bit_reference& __x) const noexcept` as well? If not, why not? (I don't see this discussed in [p2321].)  Peter Dimov answers with an educated guess: The existing `operator=(const __bit_reference&)` is only there to block the defaulted copy-assignment operator. We don't need to block `operator=(const __bit_reference&) const` because it doesn't get implicitly generated. That makes enough sense for me!



================
Comment at: libcxx/include/__bit_reference:69
+#if _LIBCPP_STD_VER > 20
+    _LIBCPP_HIDE_FROM_ABI const __bit_reference& operator=(bool __x) const noexcept {
+        if (__x)
----------------
Nit: Consider matching the brace style of lines 58–60.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117736