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

Minimal changes to support constexpr allocation in MSVC #1313

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Sep 22, 2020

  • Define _CONSTEXPR20_DYNALLOC macro to constexpr when the compiler defines __cpp_constexpr_dynamic_alloc and inline otherwise.
  • Implement and test ranges::construct_at and ranges::destroy_at, mark them (and std flavors) _CONSTEXPR20_DYNALLOC.
  • Implement ranges::destroy and ranges::destroy_n which share machinery with ranges::destroy_at with minimal test coverage. (More to follow.)

[This is a dual of internal MSVC-PR-275909.]

Partially implements #37 and #39.

* Define `_CONSTEXPR20_DYNALLOC` macro to `constexpr` when the compiler defines `__cpp_constexpr_dynamic_alloc` and `inline` otherwise.
* Implement and test `ranges::construct_at` and `ranges::destroy_at`, mark them (and `std` flavors) `_CONSTEXPR20_DYNALLOC`.
* Implement `ranges::destroy` and `ranges::destroy_n` which share machinery with `ranges::destroy_at` with minimal test coverage. (More to follow.)

[This is a dual of internal MSVC-PR-275909.]
@CaseyCarter CaseyCarter added cxx20 C++20 feature ranges C++20/23 ranges labels Sep 22, 2020
@CaseyCarter CaseyCarter requested a review from a team as a code owner September 22, 2020 04:08
@CaseyCarter CaseyCarter mentioned this pull request Sep 22, 2020
@CaseyCarter
Copy link
Member Author

Full disclosure: there's really no reason for destroy_n to be here other than that it was sitting on the same branch with the rest of this, waiting for test coverage that still isn't here. I could pull it, but I'd prefer to leave this altogether and follow it with a test coverage PR.

stl/inc/memory Show resolved Hide resolved
Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Blah blah missing test coverage, but this looks great

auto construct_at(_Ty* const _Location, _Types&&... _Args) noexcept(noexcept(::new (const_cast<void*>(
static_cast<const volatile void*>(_Location))) _Ty(_STD forward<_Types>(_Args)...))) // strengthened
_CONSTEXPR20_DYNALLOC auto construct_at(_Ty* const _Location, _Types&&... _Args) noexcept(
noexcept(::new (const_cast<void*>(static_cast<const volatile void*>(_Location)))
Copy link
Contributor

Choose a reason for hiding this comment

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

HM in the uninitialized_meow PR we introduced a voidify Funktion. If it is not too far out we should wait until it is merged or reproduce it here

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole point of pushing this through before finishing test coverage is to avoid blocking the compiler team. If we get uninit_meow merged in time, I'll make the voidify change here. Otherwise, I'll do so in the followup. (Leaving this comment open as a reminder.)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

MSVC-PR-276291 is internally out for review, containing uninit_meow, so it's a race 🚀

Copy link
Member Author

@CaseyCarter CaseyCarter Sep 22, 2020

Choose a reason for hiding this comment

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

I've annotated the destroy/destroy_n test coverage followup work item in #39 to mention integration with GH-1164 and _Voidify_iter specifically so this won't be dropped.

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

miscco commented Sep 22, 2020

Does that mean we can have a look at constexpr container after this is merged (excluding string and vector of course)

@CaseyCarter CaseyCarter merged commit 3d88a64 into microsoft:master Sep 22, 2020
@CaseyCarter CaseyCarter deleted the dynalloc branch September 22, 2020 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants