Skip to content
23 changes: 22 additions & 1 deletion stl/inc/bitset
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public:
friend bitset;

public:
_CONSTEXPR23 reference(const reference&) = default;
_CONSTEXPR23 reference(const reference& _Bitref) noexcept = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name is meaningless here.

Suggested change
_CONSTEXPR23 reference(const reference& _Bitref) noexcept = default;
_CONSTEXPR23 reference(const reference&) noexcept = default;


_CONSTEXPR23 ~reference() noexcept {} // TRANSITION, ABI

Expand All @@ -99,6 +99,11 @@ public:
return *this;
}

_CONSTEXPR23 const reference& operator=(const bool _Val) const noexcept {
_Pbitset->_Set_unchecked(_Mypos, _Val);
return *this;
}

_NODISCARD _CONSTEXPR23 bool operator~() const noexcept {
return !_Pbitset->_Subscript(_Mypos);
}
Expand All @@ -112,6 +117,22 @@ public:
return *this;
}

friend constexpr void swap(reference _Left, reference _Right) noexcept {
bool _Val = _Left; // NOT _STD swap
_Left = _Right;
_Right = _Val;
}

friend constexpr void swap(reference _Left, bool& _Right) noexcept {
bool _Val = _Left; // NOT _STD swap
_Left = _Right;
_Right = _Val;
}

friend constexpr void swap(bool& _Left, reference _Right) noexcept {
swap(_Right, _Left);
}

private:
_CONSTEXPR23 reference(bitset<_Bits>& _Bitset, const size_t _Pos) noexcept : _Pbitset(&_Bitset), _Mypos(_Pos) {}

Expand Down
17 changes: 13 additions & 4 deletions stl/inc/vector
Original file line number Diff line number Diff line change
Expand Up @@ -2461,7 +2461,7 @@ private:
using _Difference_type = typename _Mybase::_Difference_type;

public:
_CONSTEXPR20 _Vb_reference(const _Vb_reference&) = default;
_CONSTEXPR20 _Vb_reference(const _Vb_reference&) noexcept = default;

_CONSTEXPR20 _Vb_reference(const _Mybase& _Right) noexcept
: _Mybase(_Right._Myptr, _Right._Myoff, _Right._Getcont()) {}
Expand All @@ -2480,8 +2480,7 @@ public:
return *this;
}

#if _HAS_CXX23
constexpr const _Vb_reference& operator=(bool _Val) const noexcept {
_CONSTEXPR20 const _Vb_reference& operator=(bool _Val) const noexcept {
if (_Val) {
*const_cast<_Vbase*>(_Getptr()) |= _Mask();
} else {
Expand All @@ -2490,7 +2489,6 @@ public:

return *this;
}
#endif // _HAS_CXX23

_CONSTEXPR20 void flip() noexcept {
*const_cast<_Vbase*>(_Getptr()) ^= _Mask();
Expand All @@ -2517,6 +2515,16 @@ public:
_Right = _Val;
}

friend _CONSTEXPR20 void swap(_Vb_reference _Left, bool& _Right) noexcept {
bool _Val = _Left; // NOT _STD swap
_Left = _Right;
_Right = _Val;
}

friend _CONSTEXPR20 void swap(bool& _Left, _Vb_reference _Right) noexcept {
swap(_Right, _Left);
}

protected:
_CONSTEXPR20 _Vbase _Mask() const noexcept {
return static_cast<_Vbase>(1) << this->_Myoff;
Expand Down Expand Up @@ -3509,6 +3517,7 @@ public:
}
}

_DEPRECATE_VECTOR_BOOL_STATIC_REFERENCE_SWAP
static _CONSTEXPR20 void swap(reference _Left, reference _Right) noexcept {
bool _Val = _Left; // NOT _STD swap
_Left = _Right;
Expand Down
12 changes: 11 additions & 1 deletion stl/inc/yvals_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
// P3323R1 Forbid atomic<cv T>, Specify atomic_ref<cv T>
// (for atomic<cv T>)
// P3503R3 Make Type-Erased Allocator Use In promise And packaged_task Consistent
// P3612R1 Harmonize Proxy-Reference Operations

// _HAS_CXX17 directly controls:
// P0005R4 not_fn()
Expand Down Expand Up @@ -1512,7 +1513,16 @@ _EMIT_STL_ERROR(STL1004, "C++98 unexpected() is incompatible with C++23 unexpect
#define _DEPRECATE_LOCALE_EMPTY
#endif // ^^^ warning disabled ^^^

// next warning number: STL4049
#if !defined(_SILENCE_VECTOR_BOOL_STATIC_REFERENCE_SWAP_DEPRECATION_WARNING)
#define _DEPRECATE_VECTOR_BOOL_STATIC_REFERENCE_SWAP \
[[deprecated("warning STL4049: Static std::vector<bool>::swap(reference, reference) is deprecated. " \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick glance: This is a C++26 deprecation, so it should follow the pattern of the previous warnings by (1) explaining that this is deprecated in C++26 and (2) introducing/using a "silence all C++26 deprecations" macro that can be used as a coarse-grained escape hatch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I (apparently incorrectly) understood

This deprecates vector<bool>::swap(reference, reference), [...]. As this simply resolves LWG issues and doesn't take any top-level names, I believe we should implement this unconditionally [...]

as an instruction to deprecate the function regardless of C++26. Fixed, it now follows the pattern of other deprecation warnings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot about that, my cat-sized brain has a short memory (that's what I get for leaving a quick comment without deeply thinking it through).

I'll need to think about whether the deprecation should be unconditional, or guarded for C++26. I hadn't thought that through when I noticed that the message didn't explain that the deprecation was introduced in C++26 (we usually note this to distinguish Standard deprecations from Microsoft deprecations).

Making the functional changes unconditional but the deprecation guarded by C++26 could be reasonable. I'll properly review this later (I still have to get through flat_meow), thanks for the quick reaction and sorry for the randomization.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the functional changes unconditional but the deprecation guarded by C++26 could be reasonable.

Sure, that is the current state of this PR.
No worries, let me know when you decide on this matter.

"Use non-member function swap(reference, reference) instead. You can define " \
"_SILENCE_VECTOR_BOOL_STATIC_REFERENCE_SWAP_DEPRECATION_WARNING to suppress this warning.")]]
#else // ^^^ warning enabled / warning disabled vvv
#define _DEPRECATE_VECTOR_BOOL_STATIC_REFERENCE_SWAP
#endif // ^^^ warning disabled ^^^

// next warning number: STL4050

// next error number: STL1013

Expand Down
3 changes: 3 additions & 0 deletions tests/libcxx/expected_results.txt
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ std/atomics/atomics.ref/member_types.compile.pass.cpp FAIL
std/thread/futures/futures.promise/uses_allocator.pass.cpp FAIL
std/thread/futures/futures.task/futures.task.members/ctor2.compile.pass.cpp FAIL

# libc++ has not implemented P3612R1 "Harmonize Proxy-Reference Operations"
std/containers/sequences/vector.bool/reference.swap.pass.cpp FAIL

# Various bogosity (LLVM-D141004)
std/utilities/utility/mem.res/mem.res.pool/mem.res.pool.ctor/ctor_does_not_allocate.pass.cpp FAIL
std/utilities/utility/mem.res/mem.res.pool/mem.res.pool.ctor/sync_with_default_resource.pass.cpp FAIL
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

// This test was extended with functionality needed for
// P3612R1: Harmonize Proxy-Reference Operations

#include <algorithm>
#include <bitset>
#include <cassert>
#include <type_traits>
#include <vector>

using namespace std;

static const auto is_true = [](bool b) { return b; };
static const auto is_false = [](bool b) { return !b; };

int main() {
void check_values_match() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also test the functionality in constant evaluation since C++20.

E.g. we can mark this function CONSTEXPR20.

#if _HAS_CXX20
#define CONSTEXPR20
#else // ^^^ _HAS_CXX20 / !_HAS_CXX20
#define CONSEXPR20 inline
#endif // ^^^ !_HAS_CXX20 ^^^

And then write

check_values_match();
#if _HAS_CXX20
static_assert(check_values_match());
#endif // _HAS_CXX20

Same for some other functions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood; I tried to fix your comments, and a new issue surfaced: bitset<N>::reference is not (yet) a literal type in C++20 (it uses _CONSTEXPR23), while vector::reference is.

This could be solved naturally by adding #if _HAS_CXX23 to the tests, but is that the correct solution, considering the intent of the paper that both types shall have similar (or even the same?) semantics?
Making the bitset::reference _CONSTEXPR20 would require making relevant portions of bitset _CONSTEXPR20 as well though, so those changes accumulate...

I can look into it, but I need some guidance or precedent for adding constexpr to declarations that are not yet required to be constexpr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitset<N>::reference is not (yet) a literal type in C++20 (it uses _CONSTEXPR23), while vector::reference is.

We can use

#if _HAS_CXX20 && !_HAS_CXX23
if (!is_constant_evaluated())
#endif // _HAS_CXX20 && !_HAS_CXX23
{
    /* test code for bitset<N>::reference */
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point.

Nevertheless, could you please clarify what the issue would be with my suggestion (replacing about a dozen instances of _CONSTEXPR23 in <bitset> with _CONSTEXPR20)? I have done this experimentally on my local branch, and things seem to work fine (complete test suite is still running, I have hand-picked several tests touching the std::bitset).

Is it purely for maximal standards conformance? Or is constexpr part of ABI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replacing about a dozen instances of _CONSTEXPR23 in <bitset> with _CONSTEXPR20

I guess the issue is that the standard explicitly forbids adding extra constexpr to functions ([constexpr.functions]).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, other changes introduced by this PR violate [constexpr.functions] as well, don't they? e.g. the change to operator=(bool) const in std::vector<bool>::reference that @StephanTLavavej approved (see #5848 (comment))

Some degree of standards violation seems unavoidable when changes are backported... I don't mean to disagree, I'm just trying to get the correct mental model to simplify future contributions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, other changes introduced by this PR violate [constexpr.functions] as well, don't they? e.g. the change to operator=(bool) const in std::vector<bool>::reference that @StephanTLavavej approved (see #5848 (comment))

I'm not a language lawyer, but operator=(bool) const was already constexpr when it was first introduced by P2321R2, so I’d guess it doesn’t violate [constexpr.functions] (we’re just backporting the complete future functionality to an older version).

Some degree of standards violation seems unavoidable when changes are backported...

I agree, but about a dozen explicit standard violations seems a bit excessive to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.
To clarify, do you expect me to implement @frederick-vs-ja's suggestion (#5848 (comment)) or postpone the resolution? Mr. Lavavej mentioned in #5847 that he will review these two PRs later. Do we (I) want his opinion on this matter, or get only approval of the final version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing @frederick-vs-ja’s suggestion seems correct to me, but I’m not a maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, other changes introduced by this PR violate [constexpr.functions] as well, don't they? e.g. the change to operator=(bool) const in std::vector<bool>::reference that @StephanTLavavej approved (see #5848 (comment))

I personally want to leave addition of operator=(bool) const in C++23 and later. Because it doesn't seem intended for WG21-P3612R1 to backport changes of vector<bool>::reference, and it is intentional that bitset::reference should behave consistently with vector<bool>::reference (this part is also LWG-4187). IMO, as a result, operator=(bool) const of both should be restricted in C++23 and later. But maintainer(s) may have different opinions.

vector<bool> x(100, false);
vector<bool> y(100, true);

Expand All @@ -27,3 +32,50 @@ int main() {
assert(!y[34]);
assert(all_of(y.begin() + 35, y.end(), is_true));
}

template <typename T>
void check_P3612(T& collection) {

auto ref0 = collection[0];
auto const ref1 = collection[1];

// assignments from bool
ref0 = true;
ref1 = false;
// assignments from reference
ref0 = ref1;
ref1 = ref0;

collection[0] = true;
collection[1] = false;

swap(collection[0], collection[1]); // swap(reference, reference)
assert(!collection[0]);

bool b = true;
swap(collection[0], b); // swap(reference, bool)
assert(collection[0]);

swap(b, collection[0]); // swap(bool, reference)
assert(!collection[0]);
}

template <typename T>
constexpr bool has_noexcept_copy_ctor = noexcept(T(std::declval<const T&>()));

int main() {
check_values_match();


constexpr size_t N = 10;
vector<bool> vector(N);
bitset<N> bitset(0);

check_P3612(vector);
check_P3612(bitset);

static_assert(std::is_nothrow_copy_constructible_v<decltype(vector)::reference>, "");
static_assert(std::is_nothrow_copy_constructible_v<decltype(bitset)::reference>, "");
static_assert(has_noexcept_copy_ctor<decltype(vector)::reference>, "");
static_assert(has_noexcept_copy_ctor<decltype(bitset)::reference>, "");
}
3 changes: 3 additions & 0 deletions tests/tr1/tests/vector/test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
// test <vector>
#define TEST_NAME "<vector>"

// Since P3612R1 the static vector<bool>::swap(reference, reference) is deprecated
#define _SILENCE_VECTOR_BOOL_STATIC_REFERENCE_SWAP_DEPRECATION_WARNING

#include "tdefs.h"
#include <stddef.h>
#include <vector>
Expand Down