[libc++][P2321R2] Add vector<bool>::reference::operator=(bool) const

[philnik]

Summary

Add vector<bool>::reference::operator(bool) const

[• Quuxplusone]

(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!

libcxx/include/__bit_reference

69

Nit: Consider matching the brace style of lines 58–60.

[philnik]

(1) I couldn't find any tests for vector<bool>::reference. Could you tell me where they are or if there are any? If not, where should I put them?
(2) I can do that.

[• Quuxplusone]

In D117736#3257847, @philnik wrote:

(1) I couldn't find any tests for vector<bool>::reference. Could you tell me where they are or if there are any? If not, where should I put them?

git grep flip libcxx/test/ | grep -v utilities/template.bitset — no hits. So I think we don't have any tests!

They should definitely go somewhere under libcxx/test/std/containers/sequences/vector.bool/; I would think directly in that directory with hierarchical filenames like reference.flip.pass.cpp, reference.assign.pass.cpp (or maybe reference.{const_,}assign_{bool,reference}.pass.cpp, one per overload?), etc. Although I wouldn't see anything wrong with reference/flip.pass.cpp, reference/assign.pass.cpp, etc either.

[philnik]

• Add test

[• Quuxplusone]

libcxx/test/std/containers/sequences/vector.bool/reference/assign_bool.pass.cpp

40

You'll need to either hide this under TEST_STD_VER > 20 or remove the _LIBCPP_STD_VER > 20 from <__bit_reference>. Honestly, I think removing it from <__bit_reference> might be the better way to go, for consistency's sake, but I'm prepared for that to be controversial. @ldionne thoughts? can we make vector<bool>::iterator indirectly writable in C++20/17/14/11/03 as well as 2b?

46

It might be a good idea to add a more motivating test case in addition to this simple one. I'm thinking something like https://godbolt.org/z/T1f3rzejs , although it's still not crazy good as an example. I wonder if @BRevzin knows a better motivating example off the top of his head.

template<class T>
void test(T t) {
    std::vector<T> v = {t};
    const typename std::vector<T>::reference r = v[0];  // const-qualifying a reference type is a no-op
    r = t;  // therefore this should work consistently for T=int and T=bool
}

...Aha, you should also update std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp because this should now be true, not false!

static_assert(std::indirectly_writable<std::vector<bool>::iterator, bool>);

That might be a sufficiently "motivating" example.

[philnik]

libcxx/test/std/containers/sequences/vector.bool/reference/assign_bool.pass.cpp

46

Did you miss the iterator_concept_conformance.compile.pass.cpp diff? Is that enough?

[ldionne]

libcxx/test/std/containers/sequences/vector.bool/reference/assign_bool.pass.cpp

40

I would guard this with TEST_STD_VER since this is so easy to do, and that way we'll be more strictly conforming.

[philnik]

• Address commments

[ldionne]

I know it's pretty lame that this is inconsistent between C++20 and pre C++20, but I think this is the correct thing to do since we strive to remain strictly standards conforming.

libcxx/include/__bit_reference

public:
     {
         if (__x)
             *__seg_ |= __mask_;
         else
             *__seg_ &= ~__mask_;
         return *this;
     }
 
+#if _LIBCPP_STD_VER > 20
+    _LIBCPP_HIDE_FROM_ABI const __bit_reference& operator=(bool __x) const noexcept {
+        if (__x)
+            *__seg_ |= __mask_;
+        else
+            *__seg_ &= ~__mask_;
+        return *this;
+    }
+#endif
+
     _LIBCPP_INLINE_VISIBILITY
     __bit_reference& operator=(const __bit_reference& __x) _NOEXCEPT
         {return operator=(static_cast<bool>(__x));}
 
     _LIBCPP_INLINE_VISIBILITY void flip() _NOEXCEPT {*__seg_ ^= __mask_;}
     _LIBCPP_INLINE_VISIBILITY __bit_iterator<_Cp, false> operator&() const _NOEXCEPT
         {return __bit_iterator<_Cp, false>(__seg_, static_cast<unsigned>(__libcpp_ctz(__mask_)));}
 private:

libcxx/test/std/containers/sequences/vector.bool/iterator_concept_conformance.compile.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-no-concepts
 
 // iterator, const_iterator, reverse_iterator, const_reverse_iterator
 
+#include <iterator>
 #include <vector>
 
-#include <iterator>
+#include "test_macros.h"
 
 using iterator = std::vector<bool>::iterator;
 using const_iterator = std::vector<bool>::const_iterator;
 using reverse_iterator = std::vector<bool>::reverse_iterator;
 using const_reverse_iterator = std::vector<bool>::const_reverse_iterator;
 using value_type = bool;
 
 static_assert( std::random_access_iterator<iterator>);
 static_assert( std::random_access_iterator<reverse_iterator>);
 static_assert(!std::contiguous_iterator<iterator>);
 static_assert(!std::contiguous_iterator<reverse_iterator>);
+#if TEST_STD_VER > 20
+static_assert( std::indirectly_writable<iterator, value_type>);
+#else
 static_assert(!std::indirectly_writable<iterator, value_type>);
+#endif
 static_assert( std::sentinel_for<iterator, iterator>);
 static_assert( std::sentinel_for<iterator, const_iterator>);
 static_assert(!std::sentinel_for<iterator, reverse_iterator>);
 static_assert(!std::sentinel_for<iterator, const_reverse_iterator>);
 static_assert( std::sized_sentinel_for<iterator, iterator>);
 static_assert( std::sized_sentinel_for<iterator, const_iterator>);
 static_assert(!std::sized_sentinel_for<iterator, reverse_iterator>);
 static_assert(!std::sized_sentinel_for<iterator, const_reverse_iterator>);

libcxx/test/std/containers/sequences/vector.bool/reference/assign_bool.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <vector>
 
 // reference& operator=(bool)
 
 #include <cassert>
 #include <vector>
 
+#include "test_macros.h"
+
 bool test() {
   std::vector<bool> vec;
   typedef std::vector<bool>::reference Ref;
   vec.push_back(true);
   vec.push_back(false);
   Ref ref = vec[0];
   const Ref cref = vec[0];
 
   assert(ref);
   ref = false;
   assert(!vec[0]);
   assert(!vec[1]);
   ref = true;
   assert(vec[0]);
   assert(!vec[1]);
 
   assert(cref);
 
+#if TEST_STD_VER > 20
+  cref = false;
+  assert(!vec[0]);
+  assert(!vec[1]);
+  cref = true;
+  assert(vec[0]);
+  assert(!vec[1]);
+#endif
+
   return true;
 }
 
 int main(int, char**) {
   test();
 
   return 0;
 }