Match std::vector on size limits, cover the untested move path, add the C++23 range members - #100
Merged
Merged
Conversation
…ents Two things about size limits that did not match std::vector. max_size() answered PTRDIFF_MAX for every T, which is a byte count wearing a count's clothes. It claimed a size no svector could ever reach, because alloc() refuses anything whose bytes pass PTRDIFF_MAX, so the real ceiling has always been PTRDIFF_MAX/sizeof(T) -- which is what std::vector answers. Still static, for the reason the comment there already gives. And asking for more than that threw bad_alloc, where std::vector throws length_error. They are different questions: one is a size that cannot exist, the other is a size that can but that the allocator would not give. This now draws the same line std::vector does, so reserve(max_size()) is still a bad_alloc -- max_size() is a legal size and the allocation is what fails -- while one past it is a length_error. This is a visible behaviour change for anyone catching bad_alloc around a too-large insert or reserve. The two tests that spelled the old behaviour are updated rather than deleted, and there are new ones for the size error itself and for the growth path clamping at max_size() instead of wrapping.
Coverage over the header, aggregated across instantiations, had nine source lines that no test reached. One of them was svector(svector&&, Allocator const&) taking over other's allocation. Every test that reached that constructor named an allocator that did not compare equal, so it always took the other branch and moved the elements one at a time -- including the case that is a compile time yes and needs no allocator at all, which is what every user of the default std::allocator gets. That is new code in 1.3.0, in move and relocation logic, which is where this container's bugs have historically been: #54, #63, #74. It works, but nothing was checking. Now covered, in both storage modes, and the stateful cases prove the takeover rather than assume it: the ledger shows no second allocation and the elements are still at the address they started at. The unequal case is kept alongside so the contrast is visible in one place. Also covers construct_each()'s copy loop, which only runs for an allocator with a construct() of its own, and the new length_error. Three of the nine are gone. The remaining six are all provably unreachable rather than merely untested: two are LCOV_EXCL_LINE already, two are byte overflow guards that max_size() dividing by sizeof(T) now makes impossible to reach, realloc()'s direct to direct return cannot happen because reserve() only calls it when growing and shrink_to_fit() returns before it in direct mode, and calculate_new_capacity()'s wrap clamp cannot fire while max_size() is under PTRDIFF_MAX. They are cheap and they document invariants, so they stay.
The README said svector implements all of std::vector's API. C++23 added assign_range, append_range, insert_range and the from_range constructor, and libstdc++ has had them for a while, so that claim had quietly stopped being true. All four go through the iterator pair members, so a range gets the same growth, the same exception guarantees and the same self referencing checks an iterator pair already got, rather than a second implementation of all three. A range cannot always be handed over as a pair: its sentinel need not be its iterator, and its iterator need not publish an iterator_category, which is what is_input_iterator is built on. views::filter over views::iota is both. One that cannot is built into a temporary first, which costs an allocation for exactly the ranges that could not have been sized anyway. Guarded on __cpp_lib_containers_ranges rather than on the language version, because what these need is std::from_range_t and the range concepts, and a C++17 build has neither. That build is exactly what it was: 126 test cases at C++17 and C++20, 131 at C++23. The constraint is one concept spelled the way the standard spells it rather than a requires clause on each member, which also keeps clang-format from folding the clause onto the declaration. New public API, so the version goes to 1.4.0 by the rule the macros state.
The hardened leg broke on the new tests, and not for a reason about svector. max_size() answering PTRDIFF_MAX/sizeof(T) instead of PTRDIFF_MAX made it small enough for gcc to constant fold, so at -O2 it followed max_size() + 1 into reasoning about an array of that many std::string and reported an out of bounds subscript, which -Werror turned into a failure. gcc 13 then hit an internal compiler error on the same test. Nothing about that is wrong with the code under test: a size like this is a runtime value in any real use. So it is one here too, through a volatile, which is also what keeps the existing reserve_bad_alloc test out of the same trap now that its argument folds as well.
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.
Three commits, one per item, reviewable separately. Item 4 needed no code and is a comment on #79.
1.
length_errorwhere the standard sayslength_error, andmax_size()counts elementsTwo divergences from
std::vector, measured rather than assumed:std::vectorreserve(max_size()+1)length_errorbad_alloclength_errorresize(max_size()+1)length_errorbad_alloclength_errorreserve(max_size())bad_allocbad_allocbad_allocmax_size()forintPTRDIFF_MAX/4PTRDIFF_MAXPTRDIFF_MAX/4max_size()was a byte count wearing a count's clothes: it claimed a size no svector could reach, becausealloc()refuses anything whose bytes passPTRDIFF_MAX. It staysstatic, for the reason the comment there already gives.The exception split is the one
std::vectordraws: a size that cannot exist islength_error; a size that can but that the allocator will not give isbad_alloc. That is whyreserve(max_size())staysbad_alloc—max_size()is a legal size and the allocation is what fails.This is a visible behaviour change for anyone catching
bad_allocaround a too-largeinsertorreserve. The two tests that spelled the old behaviour are updated rather than deleted.I checked the obvious worry and it is not there: a count that is under
max_size()but whose byte size overflowssize_tstill throws and leaves the container intact.2. Covering the half of the extended move constructor nobody ran
Aggregated gcov over the header (lcov reports a 398% line rate for it, so this was done by hand) had 9 source lines no test reached. One was
svector(svector&&, Allocator const&)taking over other's allocation: every test that reached that constructor named an allocator that did not compare equal, so it always moved elements one at a time — including the compile-time-yes case that every user of the defaultstd::allocatorgets. That is new 1.3.0 code in move-and-relocate logic, which is where this container's bugs have historically been.Now covered in both storage modes, and the stateful cases prove the takeover rather than assume it: the ledger shows no second allocation and the elements are still at the address they started at.
Three of the nine are gone. The remaining six are provably unreachable rather than merely untested — two are already
LCOV_EXCL_LINE, two are byte-overflow guards that item 1 makes impossible to reach,realloc()'s direct-to-direct return cannot happen becausereserve()only calls it when growing andshrink_to_fit()returns before it in direct mode, andcalculate_new_capacity()'s wrap clamp cannot fire whilemax_size()is underPTRDIFF_MAX. They are cheap and they document invariants, so they stay. That also means one of my new tests was misnamed at first — it does not reach the clamp, and now says what it actually checks.3. The C++23 range members
assign_range,append_range,insert_rangeand thestd::from_rangeconstructor. The README claimed svector implements all ofstd::vector's API; libstdc++ has had these for a while, so that had quietly stopped being true.All four go through the iterator-pair members, so a range gets the same growth, exception guarantees and self-referencing checks an iterator pair already got, rather than a second implementation of all three. A range cannot always be handed over as a pair — its sentinel need not be its iterator, and its iterator need not publish an
iterator_category, which is whatis_input_iteratoris built on.views::filteroverviews::iotais both, and is tested. Those are materialised first, which costs an allocation for exactly the ranges that could not have been sized anyway.Guarded on
__cpp_lib_containers_ranges, not on the language version, because what they need isstd::from_range_t. A C++17 build is exactly what it was: 126 cases at C++17 and C++20, 131 at C++23, all green locally.New public API, so this bumps to 1.4.0 by the rule the version macros state.
Two limitations I would rather state than hide
std::from_range_t, so that code is not compiled there whatever-stdis passed. Documented in the linter. Raising the pin would cover them at the cost of re-curating the check list.sanitizersleg is C++17, so it compiles them away. The risk is low, since the new paths are thin wrappers over primitives the suite already hammers, but asanitizers-cpp23leg would close it if you want one.🤖 Generated with Claude Code