-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement P3612R1 Harmonize Proxy-Reference Operations #5848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…able outside of C++23 mode. Deprecate static swap(ref, ref)
|
Resolves #5842 |
|
There is a
(https://isocpp.org/files/papers/P3612R1.html#background , paragraph 4) Please confirm this change is OK as well (not doing it would be against the intent of the paper though as there would be an API difference between both proxy types). |
|
Making that unconditional sounds correct to me. |
|
By the way, please edit your original PR description to say "Resolves #nnnn". Only PR descriptions can set up the auto-resolve links with the magic phrases. In ordinary comments, the magic words are ignored and you just get a "mention" cross-reference. |
Done, I hope it's correct now. |
|
It is now failing because of tests of the static |
…pass.cpp as expected FAIL due to std::vector<bool>::swap(reference, reference)
|
Typically we define the silencing macro at the top of a test that has to exercise it. Search for the other deprecation macros for precedent. |
|
Oh, and please validate your changes with a local test run for any updated/affected tests, before pushing changes. This is both faster for you to iterate with, and avoids unnecessarily consuming the CI machines which are a shared resource. It's okay if everything looks fine with the limited set of tests you ran, and then you push and discover newly affected tests, but the initial local run will help avoid a lot of unnecessary delay. |
Yes, of course, I unfortunately run only std, hence I missed those two failing tests (one in libcxx and one in tr1).. Apologies for failed CIs.. |
stl/inc/bitset
Outdated
|
|
||
| public: | ||
| _CONSTEXPR23 reference(const reference&) = default; | ||
| _CONSTEXPR23 reference(const reference& _Bitref) noexcept = default; |
There was a problem hiding this comment.
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.
| _CONSTEXPR23 reference(const reference& _Bitref) noexcept = default; | |
| _CONSTEXPR23 reference(const reference&) noexcept = default; |
| static const auto is_false = [](bool b) { return !b; }; | ||
|
|
||
| int main() { | ||
| void check_values_match() { |
There was a problem hiding this comment.
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_CXX20Same for some other functions.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitset<N>::referenceis 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 */
}There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
_CONSTEXPR23in<bitset>with_CONSTEXPR20
I guess the issue is that the standard explicitly forbids adding extra constexpr to functions ([constexpr.functions]).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) constinstd::vector<bool>::referencethat @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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) constinstd::vector<bool>::referencethat @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.
stl/inc/yvals_core.h
Outdated
| #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. " \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Add friend swap functions to std::bitset::reference and std::vector::reference, add new assignment operators.
Add deprecation warning to std::vector::swap(reference, reference).
Resolves #5842