Skip to content

Implement polymorphic_allocator<> #1311

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Sep 25, 2020
Merged

Implement polymorphic_allocator<> #1311

merged 22 commits into from
Sep 25, 2020

Conversation

@onihusube
Copy link
Contributor

@onihusube onihusube commented Sep 21, 2020

@onihusube onihusube requested a review from a team as a code owner September 21, 2020 15:13
@ghost
Copy link

ghost commented Sep 21, 2020 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@CaseyCarter CaseyCarter changed the title Implemanet polymorphic_allocator<> (#10) Implement polymorphic_allocator<> Sep 21, 2020
@CaseyCarter CaseyCarter added the cxx20 C++20 feature label Sep 21, 2020
StephanTLavavej
StephanTLavavej requested changes Sep 22, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this feature! 😸 This looks solid; I have a handful of comments but they're all localized and should be straightforward to resolve.

StephanTLavavej
StephanTLavavej requested changes Sep 22, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Adding 2 comments that somehow got lost from the initial review.

onihusube and others added 11 commits September 22, 2020 13:38
@onihusube @StephanTLavavej
Update stl/inc/xpolymorphic_allocator.h
69eafeb 
Co-authored-by: Stephan T. Lavavej <[email protected]>
@onihusube @StephanTLavavej
Update stl/inc/xpolymorphic_allocator.h
c75ae36 
Co-authored-by: Stephan T. Lavavej <[email protected]>
@onihusube @StephanTLavavej
Update stl/inc/yvals_core.h
6f2b314 
Co-authored-by: Stephan T. Lavavej <[email protected]>
@onihusube
add noexcept
fd6a17d
@onihusube
use _TRY_BEGIN
7c10115
@onihusube
guard std::byte
a7703be
@onihusube
fix test order
8dd7eb5
@onihusube
fix and add test
608dd43
@onihusube
fix format
d78ef03
@StephanTLavavej
Update include directives.
fa0a388 
* Need `<cstdint>` for `std::intptr_t`.
* Need `<limits>`, not `<numeric>`, for `std::numeric_limits`.
* Need `<new>` for `std::bad_alloc` and `std::bad_array_new_length`.
@StephanTLavavej
Use std::uintptr_t for modulo.
9dc8b18
StephanTLavavej
StephanTLavavej approved these changes Sep 22, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I pushed a couple of small changes to the test:

  • Use std::uintptr_t for modulo. (Unsigned modulo is easier to reason about; signed modulo was notoriously tricky although [expr.mul] was eventually simplified in C++17 if I recall correctly.)
  • Update include directives:
    • Need <cstdint> for std::uintptr_t.
    • Need <limits>, not <numeric>, for std::numeric_limits.
    • Need <new> for std::bad_alloc and std::bad_array_new_length.

I'll move your PR forward to Final Review - then we'll just need a second STL maintainer to sign off (they tend to be active during Redmond's daytime, unlike me 😹), and then your feature will be ready to merge! 🎉

CaseyCarter
CaseyCarter suggested changes Sep 23, 2020
View reviewed changes
@SuperWig

This comment has been minimized.

Sign in to view
@onihusube

This comment has been minimized.

Sign in to view
@SuperWig

This comment has been minimized.

Sign in to view
@AdamBucior

This comment has been minimized.

Sign in to view
@SuperWig

This comment has been minimized.

Sign in to view
@StephanTLavavej StephanTLavavej self-assigned this Sep 23, 2020
StephanTLavavej
StephanTLavavej approved these changes Sep 23, 2020
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good; I am not sure why the tests timed out so I'll rerun them.

@StephanTLavavej

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@StephanTLavavej StephanTLavavej removed their assignment Sep 23, 2020
CaseyCarter
CaseyCarter approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

This looks Ready to me!

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 24, 2020

Status update: I'm planning to merge this tomorrow, or Friday at the latest - we've accumulated a few other PRs that are ready to merge, and we like to batch them up (as they have to be simultaneously merged here and our MS-internal repo, and that involves a bit of manual work).

@StephanTLavavej StephanTLavavej self-assigned this Sep 25, 2020
@StephanTLavavej StephanTLavavej merged commit c47cedc into microsoft:master Sep 25, 2020
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 25, 2020

Thanks again for implementing this feature, and congratulations on your first microsoft/STL commit! 🎉 😺 🚀

@frederick-vs-ja frederick-vs-ja mentioned this pull request Mar 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@miscco miscco miscco left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

cxx20 C++20 feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

P0339R6 polymorphic_allocator<>

6 participants

@onihusube @SuperWig @AdamBucior @StephanTLavavej @CaseyCarter @miscco