Skip to content

<deque>: Fix single-element insertion of deque#4022

Merged
StephanTLavavej merged 12 commits intomicrosoft:mainfrom
frederick-vs-ja:deque-insertion
Sep 21, 2023
Merged

<deque>: Fix single-element insertion of deque#4022
StephanTLavavej merged 12 commits intomicrosoft:mainfrom
frederick-vs-ja:deque-insertion

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 12, 2023 •
edited
Loading

As said in #1023 (comment), it's non-conforming to use rotate which requires swappability.

It seems to me that we should sometime use copy construction/assignment instead or moving, as done in vector. No, the requirements for deque are more relaxed.

Fixes #1023. Note that there'll be merge conflict with #4016.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 12, 2023 23:21
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 13, 2023
StephanTLavavej
StephanTLavavej requested changes Sep 13, 2023
View reviewed changes
@StephanTLavavej StephanTLavavej self-assigned this Sep 13, 2023
achabense
achabense reviewed Sep 14, 2023
View reviewed changes
@CaseyCarter CaseyCarter reopened this Sep 14, 2023
StephanTLavavej
StephanTLavavej reviewed Sep 18, 2023
View reviewed changes
@StephanTLavavej StephanTLavavej removed their assignment Sep 18, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 20, 2023

I thought carefully about these cases, and I think we have sufficient test coverage, so we can merge this without a second maintainer approval. I believe that resolving the merge conflict in this batch should be simple but I'll see.

@StephanTLavavej StephanTLavavej self-assigned this Sep 20, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 20, 2023

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

@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 21, 2023

I've pushed a merge with main to resolve the conflict with how #4016 moved code around. For iterator emplace(const_iterator _Where, _Valty&&... _Val), I pasted #4022's implementation in the new location, as no other changes had been made. For iterator insert(const_iterator _Where, const _Ty& _Val), both PRs made the exact same change, so I kept the new location.

@StephanTLavavej StephanTLavavej merged commit 385f133 into microsoft:main Sep 21, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 21, 2023

Thanks for fixing this bug in the STL's favorite and best container! 😹 🐞 ✅

@frederick-vs-ja frederick-vs-ja deleted the deque-insertion branch September 21, 2023 23:13
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

@achabense achabense achabense left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<deque>: std::deque::insert performance

4 participants

@frederick-vs-ja @StephanTLavavej @CaseyCarter @achabense