noexcept correctness: stop lying about moves, start promising the rest - #64
Merged
Conversation
std::vector can declare its move operations noexcept unconditionally because they only steal a pointer. An svector in direct mode has to relocate the inline elements instead, which runs T's move constructor, so the promise was not ours to make: a throwing move escaped a noexcept function and called std::terminate. Worse, std::move_if_noexcept asks exactly this trait, so growing a std::vector<svector<T>> moved the elements instead of falling back to a copy, and push_back's strong exception guarantee became a terminate. Types with a noexcept move, which is everything ordinary including std::string, std::vector, std::unique_ptr and anything trivially copyable, keep the answer they had before. boost::container::small_vector is conditional here for the same reason. While testing this, asan found that growing leaked the new storage when T's move constructor threw partway through relocating: nothing owns that allocation yet, and the element emplace_back had already built in it was never destroyed. take_over_storage now frees it on the way out. Only reachable with a throwing move, which is why the unconditional noexcept hid it. Closes #63 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
svector is meant to be a drop in for std::vector, but it made almost no noexcept promises at all: every observer, iterator accessor, clear(), pop_back() and swap() were unspecified where std::vector guarantees them. Generic code that asks, e.g. std::is_nothrow_swappable_v or a hand written if constexpr (noexcept(v.size())), got a different answer for the two types. Nothing here changes what the code does, all of these were already incapable of throwing. swap() is the one exception: it is one move construction plus two move assignments, so it inherits the condition those now carry. Not touched: at() throws out_of_range by design, and shrink_to_fit() allocates. Both match std::vector in still being able to throw. The test asserts each one against std::vector rather than against a hardcoded expectation, so the two contracts cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macos job caught that comparing every member against std::vector was the wrong test: for the parts the standard leaves open, the implementations genuinely disagree. libc++ does not declare pop_back noexcept while libstdc++ and the MSVC STL do, and libc++'s shrink_to_fit is noexcept because it swallows a failed allocation, which ours deliberately does not. So only compare the members [vector.overview] actually spells out as noexcept, where every conforming std::vector has to agree. The rest are svector's own promise and are asserted on their own. 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 #63, plus an audit of every other method.
1. The move operations promised something they cannot keep
std::vectorcan declare its movenoexceptunconditionally because it only steals a pointer. Ansvectorin direct mode has to relocate the inline elements, which runsT's move constructor. Claimingnoexceptanyway meant a throwing move calledstd::terminate, and — worse —std::move_if_noexceptasks exactly this trait, so growing astd::vector<svector<T>>moved instead of falling back to copy and turnedpush_back's strong guarantee into an abort.Both are now
noexcept(std::is_nothrow_move_constructible_v<T>).boost::container::small_vectoris conditional here for the same reason:Nothing ordinary is affected —
std::string,std::vector,std::unique_ptr, anything trivially copyable all keep the answer they had.2. A leak, found by testing the above
Once a throwing-move type could actually be tested, asan immediately caught this:
Growing allocates new storage, builds the new element into it, then relocates the existing elements. If
T's move constructor throws partway through that relocation, the new storage was owned by nothing yet and the already-built element was never destroyed — both leaked.take_over_storage()now frees them on the way out.This is pre-existing and only reachable with a throwing move, which is precisely why the unconditional
noexceptkept it hidden.3. Everything else was missing
noexceptentirelyProbing
svector<std::string, 4>againststd::vector<std::string>before this PR: every entry except the two move operations answered0wherestd::vectoranswered1—size(),capacity(),empty(),max_size(),data(), all eight iterator accessors,operator[],front(),back(),clear(),pop_back(),swap(), and the default constructor.None of them can throw; they simply never said so. After this PR the two contracts are byte-identical:
swap()is the one that needed thought — it is a move construction plus two move assignments, so it takes the same condition as those.Left alone deliberately:
at()throwsout_of_rangeby design,shrink_to_fit()allocates. Both matchstd::vectorin still being able to throw.Tests
New
test/unit/noexcept_contract.cpp. The contract assertions compare againststd::vectorrather than a hardcoded expectation, so the two cannot drift apart, and the macro stringifies the member so a failure says which one:Plus four runtime cases: a throwing move now unwinds instead of terminating (construction and assignment), an indirect move still never touches the elements,
std::vectorgrowth keeps the strong guarantee, and the leak above is caught under asan.Verified green with gcc and clang under
-fsanitize=address,undefined. Benchmarked the growth path A/B — the addedtry/catchcosts nothing measurable, as expected for table-based EH.