Fix resize() leaking when an element constructor throws - #77
Merged
Conversation
resize_after_reserve() built the new elements in a hand written loop and only recorded the new size once the whole loop had finished. A constructor throwing halfway left everything already built unreachable: the size still said current_size, so nothing ever destroyed them. Rather than wrapping the loop in a try/catch, the loop is gone. Both std::uninitialized_value_construct_n and std::uninitialized_fill_n destroy what they built before rethrowing, which is exactly the missing behaviour, and it is already why insert() constructs through them instead of looping. Value construction rather than default construction, so resize(n) on an svector<int> keeps zeroing the new elements the way T() did. Reachable through resize(count), resize(count, value), svector(count) and svector(count, value). Reverting it leaks the 450 objects the new tests build. Closes #70 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 #70.
resize_after_reserve()constructed the new elements in a loop and only recorded the new size once the loop had finished. If element k threw, elementscurrent_size..kwere alive but the size never moved, so nothing ever destroyed them.Exactly the 5 elements that had been constructed.
Fix
My first attempt wrapped the loop in a
try/catch. The review pushed back from four directions and it was right: the loop should not exist.std::uninitialized_value_construct_nandstd::uninitialized_fill_nboth destroy what they built before rethrowing, which is precisely the missing behaviour — and it is already whyinsert()constructs through them instead of looping.fill_uninitialized_space's own doc comment says so.So the fix deletes code rather than adding it, and
resize_after_reservestops being the one place in the file that hand-rolls what the rest gets from the standard algorithms.One trap worth naming: two of the four reviewers suggested
std::uninitialized_default_construct_n. That would have been a silent regression — it default-initializes, soresize(n)on ansvector<int>would leave the new elements holding garbage instead of zeroing them the wayT()does.value_constructis the one that matches. Verified directly:Guarantee
Size and element values are unchanged on failure. Capacity may already have grown, because
reserve()runs before the construction —std::vector::resizepromises the strong guarantee including capacity, so this is the basic guarantee plus "size and values unchanged", not literally strong. That ordering predates this change.Tests
Three cases in
test/unit/resize.cppsweeping start sizes 0-6 × target sizes up to 12 × first/last throw point, acrossresize(count),resize(count, value)andsvector(count). Reverting to the loop leaks 450 objects under LSan.Cost
Compared
-O3codegen before and after:.textidentical at 9749 bytes, no.gcc_except_tablein either, and theintfill still vectorizes to the samemovupssequence.Audit
Checked every other placement-new in the header.
emplace_back_growhas its own guard,emplace_backconstructs before committing the size, and the three single-elementinsert/emplacesites go throughfill_uninitialized_space. This was the last multi-element construction loop without cleanup.