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

Fix unnecessary self-move-assignments on calling deque erase(x,x) #1148

Merged
merged 7 commits into from
Aug 9, 2020

Conversation

Arzaghi
Copy link
Contributor

@Arzaghi Arzaghi commented Aug 4, 2020

Fixes issue #1118
I think it's the simplest method to solve the issue. however, it adds a little bit overhead to the library with an additional IF.
Any better idea?

@miscco
Copy link
Contributor

miscco commented Aug 4, 2020

I believe the "conventional" solution would be to simply add

if (_First == _Last) {
    return _First;
}

at the beginning of the function.

@Arzaghi Arzaghi marked this pull request as ready for review August 4, 2020 20:22
@Arzaghi Arzaghi requested a review from a team as a code owner August 4, 2020 20:22
@miscco
Copy link
Contributor

miscco commented Aug 4, 2020

Also note, that formatting seems to be off. You need to use clang-format 10 with the file setting

@Arzaghi
Copy link
Contributor Author

Arzaghi commented Aug 4, 2020

I believe the "conventional" solution would be to simply add

if (_First == _Last) {
    return _First;
}

at the beginning of the function.

It adds an additional IF statement to the code as well. This IF statement will be effective in just a rare call.
It would be perfect if the algorithm handles the rare situation naturally.

@CaseyCarter CaseyCarter added the bug Something isn't working label Aug 4, 2020
stl/inc/deque Outdated Show resolved Hide resolved
stl/inc/deque Outdated Show resolved Hide resolved
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 fixing this!

I wonder whether we should have test coverage for this, but we can file a separate issue for that.

stl/inc/deque Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Aug 8, 2020
@StephanTLavavej StephanTLavavej merged commit 1913566 into microsoft:master Aug 9, 2020
@StephanTLavavej
Copy link
Member

Thanks for fixing this bug, and congratulations on your first microsoft/STL commit! 🎉 😸

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>: erase with two iterators invokes move / move_backward when the range is empty
5 participants