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

[philnik]

Summary

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

[Mordante]

libcxx/include/bitset

717–722

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

30

FYI you can use ASSERT_SAME_TYPE.

30

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

31

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

50–59

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

16 ↗

(On Diff #419682)

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

50–59

This comment doesn't seem to be addressed.

[philnik]

• Address comments

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

16 ↗

(On Diff #419682)

This test should become constexpr once we implement P1004R2.

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

50–59

Thanks! I completely forgot to remove the comment.

[var-const]

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

16 ↗

(On Diff #419682)

In that case, can you please add a TODO?

[philnik]

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

16 ↗

(On Diff #419682)

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

2086 ↗

(On Diff #419939)

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_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_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/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>();
 
     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 <type_traits>
 
 #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)
+#if _LIBCPP_ABI_VERSION >= 2
+    ASSERT_SAME_TYPE(decltype(cases[0][0]), bool);
+#else
+    ASSERT_SAME_TYPE(decltype(cases[0][0]), typename std::bitset<N>::const_reference);
+#endif
+#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>();
 
     return 0;
 }