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

Prepare the constexpr machinery needed for string and vector #1546

Merged
merged 16 commits into from
Feb 3, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Dec 21, 2020

This is a precursor for #1407 and #1502

The hope is that this can more easily flesh out the clang bugs

@miscco miscco requested a review from a team as a code owner December 21, 2020 12:40
@miscco miscco force-pushed the constexpr_machinery branch 15 times, most recently from 6e347e9 to cb4f529 Compare December 24, 2020 20:14
@mnatsuhara mnatsuhara self-assigned this Dec 31, 2020
@StephanTLavavej StephanTLavavej added the cxx20 C++20 feature label Jan 5, 2021
@miscco
Copy link
Contributor Author

miscco commented Jan 8, 2021

I rebased this upon the latest changes and included the mutable fix.

stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jan 11, 2021

I believe we need a new define.

Once the next preview is out, we want to switch this around and activate all the machinery for MSVC only due to the clang bug that prevents construction of mutable subobjects during constant evaluation.

At the same time we should not deactivate the machinery for allocations during constant evalutions.

My proposal would be to move to _CONSTEXPR20_CONTAINER and use this going forward. Thoughts?!

@mnatsuhara
Copy link
Contributor

I think you're right. To recap the situation:
Once we can consume 16.9 Preview 3, we should (hopefully 🤞) be unblocked to implement constexpr std::string and std::vector, so _CONSTEXPR20_DYNALLOC should be set to constexpr as long as we are in C++20 mode (not restricted by any specific compiler), and inline otherwise.

However, even once we have the next preview, we will still be blocked for Clang by Bug 48606 so we want some new macro that will be constexpr if _HAS_CXX20 && defined(_MSC_VER), and inline otherwise.

The only way that we could use _CONSTEXPR20_DYNALLOC for the constexpr containers would be to relinquish Clang support for WG21-P0784, which does seem undesirable. We'll just need to be careful trying to juggle yet another constexpr-related macro! _CONSTEXPR20_CONTAINER seems fine to me.

stl/inc/xutility Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
Base automatically changed from master to main January 28, 2021 00:35
Copy link
Contributor

@mnatsuhara mnatsuhara left a comment

Choose a reason for hiding this comment

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

I did a full review of the changes, and I think we're in pretty good shape! Just a few small changes and one potential bug (in _Uninitialized_copy_unchecked)!

stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Feb 2, 2021

Awesome, thanks a lot!

@StephanTLavavej StephanTLavavej self-assigned this Feb 2, 2021
stl/inc/xmemory Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Feb 3, 2021
@StephanTLavavej StephanTLavavej self-assigned this Feb 3, 2021
@StephanTLavavej
Copy link
Member

This is blocked by DevCom-1328628 "SFINAE bug when a default argument is protected", encountered when mirroring this to the MSVC-internal repo. (Part of MSVC uses std::optional with RapidJSON - std::optional uses the constrained construct_at here, while RapidJSON contains the protected static const data member pattern.)

I may be able to prepare a workaround.

@StephanTLavavej StephanTLavavej merged commit 47cd703 into microsoft:main Feb 3, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing this machinery and finding so many bugs in so many compilers! 😹 ⚙️ 🪲 🪲 🪲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants