[libc++] `bitset::operator[] const` should return bool

[philnik]

Summary

Fixes https://github.com/llvm/llvm-project/issues/10686

[Mordante]

libcxx/include/bitset

718–723

Changing the return type is an ABI break. So we can only make this change in the unstable ABI.

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

28

FYI you can use ASSERT_SAME_TYPE.

28

Since the operator returns different types, maybe it's good to add an explicit type test in
libcxx/test/std/utilities/template.bitset/bitset.members/index.pass.cpp too.

[philnik]

• Address comments

[Mordante]

Thanks for the update, only a few small issues remain.

libcxx/include/__config

118

Let's make clear it's about the const overload.

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

29

Typically we just use the ABI name in our tests, for example in test/std/re/re.const/re.synopt/syntax_option_type.pass.cpp

#ifdef _LIBCPP_ABI_REGEX_CONSTANTS_NONZERO  // https://llvm.org/PR35967
    assert(std::regex_constants::ECMAScript != 0);
#else
    assert(std::regex_constants::ECMAScript == 0);
#endif

(In the WIP script to test against internal macro's I've whitelisted _LIBCPP_ABI_*.)

I actually think the test should be slightly different

#if !defined(_LIBCPP_VERSION) || defined(_LIBCPP_ABI_REGEX_CONSTANTS_NONZERO)  // https://llvm.org/PR35967
    assert(std::regex_constants::ECMAScript != 0);
#else
    assert(std::regex_constants::ECMAScript == 0);
#endif

[philnik]

• Address comments

[Mordante]

LGTM!

[jloser]

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

15

I don't think this include is needed since we're using ASSERT_SAME_TYPE macro now.

[ldionne]

What an annoying "extension". The intent is good, however it results in our implementation being clearly non-conforming. I'm in favour of this, and it's a shame that it has to be guarded behind an ABI check.

Can you please add a test that checks for:

typename Container::const_reference cr0 = c[0];
c[0] = 1;
assert(cr0 == 1);

at runtime (and a corresponding test case for when const_reference is bool)?

[philnik]

• Address comments

[philnik]

• Try to fix CI

[ldionne]

LGTM with changes, thanks!

libcxx/include/__config

117

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

46–55

Can you add the same test to the non-const operator[] (but you'll only have one assert)?

Also, I'd recommend against using #endif // comment since this block is so short -- in this case it's even wrong.

[ldionne]

It looks like we have the same issue in vector<bool> -- can we also fix it there? I think we only need one macro for both, so I'd make that change in this patch. Sorry for the churn.

[Mordante]

In D122092#3414576, @ldionne wrote:

It looks like we have the same issue in vector<bool> -- can we also fix it there? I think we only need one macro for both, so I'd make that change in this patch. Sorry for the churn.

vector<bool> returns a reference` or a const_reference as expected (https://en.cppreference.com/w/cpp/container/vector/operator_at)
Or am I missing something?

[philnik]

• Address comments

@Mordante I didn't see it either. const_reference should be defined as bool.

[philnik]

• Address more comments

[var-const]

libcxx/test/std/containers/sequences/vector.bool/const_reference.pass.cpp

17

Optional nit: since this test isn't constexpr, it doesn't need to return anything -- unless you expect it to become constexpr at some future point?

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

46–55

This comment doesn't seem to be addressed.

[philnik]

• Address comments

libcxx/test/std/containers/sequences/vector.bool/const_reference.pass.cpp

17

This test should become constexpr once we implement P1004R2.

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.pass.cpp

46–55

Thanks! I completely forgot to remove the comment.

[var-const]

libcxx/test/std/containers/sequences/vector.bool/const_reference.pass.cpp

17

In that case, can you please add a TODO?

[philnik]

libcxx/test/std/containers/sequences/vector.bool/const_reference.pass.cpp

17

I don't think adding a TODO makes a lot of sense here. When implementing P1004R2 almost every single file inside vector.bool and vector have to be changed anyways.

[philnik]

• Fix CI

[ldionne]

LGTM once CI is passing. Please ping me again if it ends up being more complicated than just removing the now redundant insert() method. Thanks for the cleanup!

libcxx/include/vector

2088

To fix the CI issue, I think you'll need to remove the additional insert(...) method we have that takes a const_reference. It's not in the spec, and it was probably just provided to make it work with our non-conforming const_reference type.

You should also survey for other places where we take a const_reference and make sure that those methods are all part of the spec -- it's possible that we have other cases like this, and we might not catch them (I don't know how good our coverage is for std::vector<bool>).

libcxx/test/std/utilities/template.bitset/bitset.members/index.pass.cpp

61–64

[philnik]

• Remove insert overload

@ldionne I just removed the insert overload, but it seems it was never defined, so I remove it in ABIv1 too.

[philnik]

• Try to fix CI

[ldionne]

LGTM, but I would really like to investigate removing const_reference from bitset altogether, since there isn't one in the Standard. It's a rather bad API to provide a typedef called const_reference that isn't a reference, and in fact it leads to the very confusion we are removing an extension for. I'd rather break people's source (considering there probably isn't too many users of bitset::const_reference out there) than give them an API that is both non-standard and error-prone.

[rsmith]

Is it intentional that this leaves std::vector<bool>::const_iterator::reference as the class type? We're seeing some breakage in absl due to that.

We're also seeing some breakage due to the result of std::bitset::operator[]() const no longer being convertible to std::bitset::const_reference, which would at least have been easier to understand if the const_reference typedef were removed entirely.

[rsmith]

In D122092#3452383, @rsmith wrote:

Is it intentional that this leaves std::vector<bool>::const_iterator::reference as the class type? We're seeing some breakage in absl due to that.

FWIW, the broken code looks roughly like this:

struct NumberLike {
  // ...
  template<typename T> NumberLike(T) requires std::is_arithmetic_v<T>;
  NumberLike(std::vector<bool>::reference);
  NumberLike(std::vector<bool>::const_reference);
}
void Consume(NumberLike);
template<typename Container>
void ConsumeAll(const Container &c) {
  for (const auto &v : c) Consume(v);
}

This works if vector<bool>::const_reference and vector<bool>::const_iterator::reference are the same type, but if the iterator produces the class type but vector<bool>::const_reference is simply bool then the Consume call fails.

[philnik]

In D122092#3452383, @rsmith wrote:

Is it intentional that this leaves std::vector<bool>::const_iterator::reference as the class type? We're seeing some breakage in absl due to that.

We're also seeing some breakage due to the result of std::bitset::operator[]() const no longer being convertible to std::bitset::const_reference, which would at least have been easier to understand if the const_reference typedef were removed entirely.

I just uploaded D123851 for the vector part. I'm planning to look into removing bitset::const_reference, but since it shouldn't exist, nobody should use it anyways.

libcxx/include/__config

 // std::random_device, one needs to retain the same object layout to remain ABI
 // compatible. This switch removes these workarounds for platforms that don't care
 // about ABI compatibility.
 #  define _LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT
 // Remove basic_string common base
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 // Remove vector base class
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_VECTOR_BASE_COMMON
+// According to the Standard, `bitset::operator[] const` returns bool
+#  define _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL
 #elif _LIBCPP_ABI_VERSION == 1
 #  if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with
 // COFF dllexport semantics, since dllexport forces a non-inline definition
 // of inline functions to be emitted anyway. Our own non-inline copy would
 // conflict with the dllexport-emitted copy, so we disable it.
 #    define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS

libcxx/include/bitset

public:
     bitset& reset(size_t __pos);
     _LIBCPP_INLINE_VISIBILITY
     bitset  operator~() const _NOEXCEPT;
     _LIBCPP_INLINE_VISIBILITY
     bitset& flip() _NOEXCEPT;
     bitset& flip(size_t __pos);
 
     // element access:
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
-                              const_reference operator[](size_t __p) const {return base::__make_ref(__p);}
-    _LIBCPP_INLINE_VISIBILITY       reference operator[](size_t __p)       {return base::__make_ref(__p);}
+#ifdef _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR            bool operator[](size_t __p) const {return base::__make_ref(__p);}
+#else
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR const_reference operator[](size_t __p) const {return base::__make_ref(__p);}
+#endif
+    _LIBCPP_HIDE_FROM_ABI                         reference operator[](size_t __p)       {return base::__make_ref(__p);}
     _LIBCPP_INLINE_VISIBILITY
     unsigned long to_ulong() const;
     _LIBCPP_INLINE_VISIBILITY
     unsigned long long to_ullong() const;
     template <class _CharT, class _Traits, class _Allocator>
         basic_string<_CharT, _Traits, _Allocator> to_string(_CharT __zero = _CharT('0'),
                                                             _CharT __one = _CharT('1')) const;
     template <class _CharT, class _Traits>

libcxx/include/vector

private:
     typedef typename __storage_traits::pointer       __storage_pointer;
     typedef typename __storage_traits::const_pointer __const_storage_pointer;
 
     __storage_pointer                                      __begin_;
     size_type                                              __size_;
     __compressed_pair<size_type, __storage_allocator> __cap_alloc_;
 public:
     typedef __bit_reference<vector>                  reference;
+#ifdef _LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL
+    using const_reference = bool;
+#else
     typedef __bit_const_reference<vector>            const_reference;
+#endif
 private:
     _LIBCPP_INLINE_VISIBILITY
     size_type& __cap() _NOEXCEPT
         {return __cap_alloc_.first();}
     _LIBCPP_INLINE_VISIBILITY
     const size_type& __cap() const _NOEXCEPT
         {return __cap_alloc_.first();}
     _LIBCPP_INLINE_VISIBILITY
     __storage_allocator& __alloc() _NOEXCEPT
         {return __cap_alloc_.second();}

 #if _LIBCPP_STD_VER > 11
     template <class... _Args>
    _LIBCPP_INLINE_VISIBILITY iterator emplace(const_iterator position, _Args&&... __args)
         { return insert ( position, value_type ( _VSTD::forward<_Args>(__args)... )); }
 #endif
 
     iterator insert(const_iterator __position, const value_type& __x);
     iterator insert(const_iterator __position, size_type __n, const value_type& __x);
-    iterator insert(const_iterator __position, size_type __n, const_reference __x);
     template <class _InputIterator>
         typename enable_if
         <
              __is_cpp17_input_iterator  <_InputIterator>::value &&
             !__is_cpp17_forward_iterator<_InputIterator>::value,
             iterator
         >::type
         insert(const_iterator __position, _InputIterator __first, _InputIterator __last);
template <class _ForwardIterator>
             void
         >::type
         __construct_at_end(_ForwardIterator __first, _ForwardIterator __last);
     void __append(size_type __n, const_reference __x);
     _LIBCPP_INLINE_VISIBILITY
     reference __make_ref(size_type __pos) _NOEXCEPT
         {return reference(__begin_ + __pos / __bits_per_word, __storage_type(1) << __pos % __bits_per_word);}
     _LIBCPP_INLINE_VISIBILITY
-    const_reference __make_ref(size_type __pos) const _NOEXCEPT
-        {return const_reference(__begin_ + __pos / __bits_per_word, __storage_type(1) << __pos % __bits_per_word);}
+    const_reference __make_ref(size_type __pos) const _NOEXCEPT {
+        return __bit_const_reference<vector>(__begin_ + __pos / __bits_per_word,
+                                             __storage_type(1) << __pos % __bits_per_word);
+    }
     _LIBCPP_INLINE_VISIBILITY
     iterator __make_iter(size_type __pos) _NOEXCEPT
         {return iterator(__begin_ + __pos / __bits_per_word, static_cast<unsigned>(__pos % __bits_per_word));}
     _LIBCPP_INLINE_VISIBILITY
     const_iterator __make_iter(size_type __pos) const _NOEXCEPT
         {return const_iterator(__begin_ + __pos / __bits_per_word, static_cast<unsigned>(__pos % __bits_per_word));}
     _LIBCPP_INLINE_VISIBILITY
     iterator __const_iterator_cast(const_iterator __p) _NOEXCEPT

libcxx/test/std/containers/sequences/vector.bool/const_reference.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>
+
+#include <cassert>
+#include <vector>
+
+#include "test_macros.h"
+
+bool test() {
+  using CRefT = std::vector<bool>::const_reference;
+#if !defined(_LIBCPP_VERSION) || defined(_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL)
+  ASSERT_SAME_TYPE(CRefT, bool);
+#else
+  ASSERT_SAME_TYPE(CRefT, std::__bit_const_reference<std::vector<bool> >);
+  std::vector<bool> vec;
+  vec.push_back(true);
+  CRefT ref = vec[0];
+  assert(ref);
+  vec[0] = false;
+  assert(!ref);
+#endif
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+
+  return 0;
+}

libcxx/test/std/utilities/template.bitset/bitset.members/index.pass.cpp

for (std::size_t c = 0; c != cases.size(); ++c) {
             bool b = ~r;
             assert(r == true);
             assert(v1.test(N/2) == true);
             assert(b == false);
             r.flip();
             assert(r == false);
             assert(v1.test(N/2) == false);
         }
+        ASSERT_SAME_TYPE(decltype(v1[0]), typename std::bitset<N>::reference);
     }
 }
 
 int main(int, char**) {
     test_index<0>();
     test_index<1>();
     test_index<31>();
     test_index<32>();
     test_index<33>();
     test_index<63>();
     test_index<64>();
     test_index<65>();
     test_index<1000>();
 
+    std::bitset<1> set;
+    set[0] = false;
+    auto b = set[0];
+    set[0] = true;
+    assert(b);
+
     return 0;
 }

libcxx/test/std/utilities/template.bitset/bitset.members/index_const.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
 //
 //===----------------------------------------------------------------------===//
 
 // test constexpr bool operator[](size_t pos) const;
 
 #include <bitset>
 #include <cassert>
 #include <cstddef>
 #include <vector>
 
 #include "../bitset_test_cases.h"
 #include "test_macros.h"
 
 template <std::size_t N>
 void test_index_const() {
     std::vector<std::bitset<N> > const cases = get_test_cases<N>();
     for (std::size_t c = 0; c != cases.size(); ++c) {
         std::bitset<N> const v = cases[c];
         if (v.size() > 0) {
             assert(v[N/2] == v.test(N/2));
         }
     }
+#if !defined(_LIBCPP_VERSION) || defined(_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL)
+    ASSERT_SAME_TYPE(decltype(cases[0][0]), bool);
+#else
+    ASSERT_SAME_TYPE(decltype(cases[0][0]), typename std::bitset<N>::const_reference);
+#endif
 }
 
 int main(int, char**) {
     test_index_const<0>();
     test_index_const<1>();
     test_index_const<31>();
     test_index_const<32>();
     test_index_const<33>();
     test_index_const<63>();
     test_index_const<64>();
     test_index_const<65>();
     test_index_const<1000>();
 
+  std::bitset<1> set_;
+  set_[0] = false;
+  const auto& set = set_;
+  auto b = set[0];
+  set_[0] = true;
+#if !defined(_LIBCPP_VERSION) || defined(_LIBCPP_ABI_BITSET_VECTOR_BOOL_CONST_SUBSCRIPT_RETURN_BOOL)
+  assert(!b);
+#else
+  assert(b);
+#endif
+
     return 0;
 }