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

Cleanups for <deque> part 1 #4016

Merged
merged 20 commits into from
Sep 21, 2023
Merged

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Sep 8, 2023

Contents of this pr:

  1. trivial relocations XXX: The definition order of deque's methods was highly chaotic; reorder them to roughly match with the standard.
  2. A&B: some minor cleanups for push/insert functions
  3. C: Let _Construct[_n] do proxy allocation to save some lines.
  4. D: The usage of _Allocate_at_least_helper in _Growmap was problematic, as it can make _Mapsize() not power of 2 if _Allocate_at_least_helper changes _Newsize; restore to the old version. moved to pr 4017

@achabense achabense requested a review from a team as a code owner September 8, 2023 14:10
@achabense achabense closed this Sep 8, 2023
@achabense achabense changed the title Cleanups for Cleanups for <deque> part 1 Sep 8, 2023
@achabense achabense reopened this Sep 8, 2023
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Sep 8, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 8, 2023
@StephanTLavavej
Copy link
Member

Thanks! The fine-grained commits, combined with VSCode's new ability to identify moved code, made this easy to review. Everything looks good and I didn't find any leftover redundant access specifiers.

@StephanTLavavej StephanTLavavej removed their assignment Sep 20, 2023
@StephanTLavavej
Copy link
Member

Despite the magnitude of the edits, the well-structured fine-grained commits made this very easy to review, and I am confident that we don't need a second maintainer approval.

@StephanTLavavej StephanTLavavej self-assigned this Sep 20, 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 7ffed1d into microsoft:main Sep 21, 2023
@StephanTLavavej
Copy link
Member

Thanks for making <deque> match the Standard more obviously! 🎉 😸 🚀

@achabense achabense deleted the _Deque_cleanups branch September 22, 2023 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants