Fix erase() of an empty range blanking the tail - #72
Merged
Conversation
erase_checked_end ran std::move(erase_end, container_end, erase_begin) unconditionally. When first == last the destination equals the source begin, which std::move does not allow, and it self-move-assigns every element from there to the end. With std::string they all come back empty. Same class of bug as #65, and grepping the header for the assigning algorithms shows these were the only two: every other data movement goes through std::uninitialized_move or std::uninitialized_copy, which construct into raw memory and cannot self-assign. The guard reuses num_erased, hoisted above it, rather than comparing the pointers a second way. make_long_strings() moves to VecTester.h, since #65's test grew the same "strings long enough to defeat SSO" helper in insert.cpp. Closes #66 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #66. Same class of bug as #65, different function.
v.erase(it, it)blanked every element fromitonwards:erase_checked_endranstd::move(erase_end, container_end, erase_begin)unconditionally. Withfirst == lastthe destination equals the source begin, whichstd::movedoes not allow, and every element self-move-assigns.This was the last one
Grepped the whole header for the assigning algorithms. There are exactly two raw call sites:
std::move(...)inerase_checked_endstd::move_backward(...)inshift_rightEvery other data movement goes through
std::uninitialized_move/std::uninitialized_copy/std::uninitialized_fill_n, which construct into raw memory and so cannot self-assign. The remainingstd::movehits are the single-object cast, not the algorithm. So this bug class is now fully covered.Depth
The guard belongs in
erase_checked_end, not the publicerase():tois clamped againstcontainer_endfirst, so a range can be non-empty on entry and still clamp to empty. Checkingfirst == lastin the caller would miss that case, and re-deriving the clamp there would duplicate the logic.It reuses
num_erased(hoisted above the guard) instead of spelling "nothing to erase" a second way as a pointer comparison.Tests
One case in
test/unit/erase.cpp, covering begin/middle/end positions in both storage modes. Fails without the guard.make_long_strings()moved intotest/app/VecTester.h— #65's test grew the same "strings long enough to defeat SSO" scaffolding ininsert.cpp, and this would have been the second copy. Both now call the helper.Verified with gcc and clang under
-fsanitize=address,undefined.