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

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

Merged
merged 12 commits into from
Sep 21, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 12, 2023

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 self-assigned this Sep 13, 2023
stl/inc/deque Outdated Show resolved Hide resolved
stl/inc/deque Outdated Show resolved Hide resolved
stl/inc/deque Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter reopened this Sep 14, 2023
@StephanTLavavej StephanTLavavej removed their assignment Sep 18, 2023
@StephanTLavavej
Copy link
Member

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

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

@StephanTLavavej
Copy link
Member

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
35 checks passed
@StephanTLavavej
Copy link
Member

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
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<deque>: std::deque::insert performance
4 participants