Skip to content
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

Implement LWG-3870 Remove voidify #3475

Merged
merged 44 commits into from Mar 28, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Feb 17, 2023

Fixes #3431.

IIUC static_cast<void*>(p) is OK for most of original uses of _Voidify_iter, since the arguments are generally raw pointers.

The original _Voidify_iter is still valuable for allocate_shared(_for_overwrite) due to fancy pointers, so this PR changes its name to _Voidify_unfancy (with constexpr dropped), moves it to <memory>, and makes it available only when _HAS_CXX20.

The only One remain exception is the use in allocator::construct (removed in C++20), since N4659 [depr.default.allocator]/6 said that cv-qualification is dropped.

Edit: make_shared(_for_overwrite) seemly needs cv-removing mechanisms.

I have no idea how to add test coverage since this only increases hard errors. On the contrary, we need to drop some invalidated test cases.

The test code added in #3396 is invalidated. I have to implement test coverage in another way.


There seem some breaking and pre-existing inconsistency in optional, variant, expected, _Variantish, _Movable_box, and _Defaultabox. I'm trying to fix them.

I tried to fix them except for expected. An LWG issue about expected is filed, I'll speculatively implement the proposed resolution. That is LWG-3891 but isn't speculatively implemented here.


Now this PR also speculative implements proposed resolutions of 1 LWG issue:

  • LWG-3888 Most ranges uninitialized memory algorithms are underconstrained

@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Feb 17, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in Code Reviews Feb 17, 2023
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Feb 17, 2023
@StephanTLavavej StephanTLavavej removed their assignment Mar 19, 2023
@StephanTLavavej
Copy link
Member

FYI @strega-nil-ms I pushed changes after you approved - all cleanup-level except for a typo bugfix.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Mar 19, 2023
@frederick-vs-ja
Copy link
Contributor Author

LWG-3901 is set to Tentatively NAD, while [mem.poly.allocator.mem]/15 says the plain "by uses-allocator construction" instead of uninitialized_construct_using_allocator, so it seems that polymorphic_allocator::construct is still required to support construction for cv-qualified types.

Should I restore the support?

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Work In Progress in Code Reviews Mar 23, 2023
@StephanTLavavej
Copy link
Member

@frederick-vs-ja If I understand the issue correctly, then yes - restoring the support sounds correct. I was about to add this to the final merge batch for 17.7 Preview 1 but I'd rather iterate on this again rather than merge something that has to be corrected later. (Changes will still flow into 17.7 Preview 2 after this.)

@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Mar 23, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 24, 2023
@timsong-cpp
Copy link
Contributor

LWG-3901 is set to Tentatively NAD, while [mem.poly.allocator.mem]/15 says the plain "by uses-allocator construction" instead of uninitialized_construct_using_allocator, so it seems that polymorphic_allocator::construct is still required to support construction for cv-qualified types.

If you actually asked that question - about polymorphic_allocator::construct specifically rather than "uses-allocator construction" generally - LWG's answer may well have been different. Certainly mine would be - the allocator requirements do not require construct to support cv-qualified types, and I have no problem with changing polymorphic_allocator::construct.

@StephanTLavavej StephanTLavavej removed their assignment Mar 24, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Mar 24, 2023
@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Mar 27, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

If LWG changes polymorphic_allocator in the future, we can again rip out support for constructing cv-qualified types, I'm not super concerned about it.

@StephanTLavavej StephanTLavavej self-assigned this Mar 28, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 6645bc3 into microsoft:main Mar 28, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Mar 28, 2023
@StephanTLavavej
Copy link
Member

Thanks for implementing this major LWG issue!

🐈‍⬛ 🐈‍⬛ 🐈‍⬛

@frederick-vs-ja frederick-vs-ja deleted the lwg-3870 branch March 28, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LWG Library Working Group issue
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

LWG-3870 Remove voidify
5 participants