Repository navigation
Fix VLA rollback after partial construction - #215
Merged
thirtytwobits merged 4 commits intoOct 6, 2026
Merged
thirtytwobits merged 4 commits into
thirtytwobits merged 4 commits into
Conversation
Reuse the uninitialized_move rollback machinery with allocator-aware lifetime operations and clean up abandoned allocations. Cover constructor, relocation, resize, assignment, and packed-bool failure paths. #verification #docs
thirtytwobits
added this pull request to stack #216
October 6, 2026 02:11
Remove the pf17 memory header dependency and track completed constructions directly in the VLA base. Cover allocator hook failures, source access failures, and empty construction ranges through VLA tests. #verification #docs
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The cleanup paths are coherent, exception-disabled builds remain gated, and the regression coverage is extensive.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes VLA exception safety during partial construction and relocation.
Changes:
- Rolls back constructed elements and abandoned storage on failure.
- Preserves allocator-aware destruction and container bookkeeping.
- Adds comprehensive exception-safety regression tests.
| File | Description |
|---|---|
include/cetl/variable_length_array.hpp |
Implements rollback and cleanup logic. |
cetlvast/suites/unittest/test_variable_length_array_exception_safety.cpp |
Tests failure paths, lifetimes, and reuse. |
cetlvast/suites/unittest/CMakeLists.txt |
Registers the new test suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pavel-kirienko
approved these changes
Oct 6, 2026
thirtytwobits
added a commit
that referenced
this pull request
Oct 6, 2026
`reserve()` and `shrink_to_fit()` could pass live non-trivially-copyable elements to an allocator's `reallocate`, which may move the block by copying bytes. Address-sensitive elements then retained pointers into released storage, and element constructors/destructors were bypassed. Gate the shared reallocation helper on `std::is_trivially_copyable<value_type>`. Other elements use the existing allocator-aware relocation and rollback from #215. Trivial storage and packed bool retain reallocation support. Non-trivial elements now require replacement storage for capacity changes, including when the underlying resource could resize in place; the public documentation explains reserving capacity up front for single-allocation resources. Stacked on #215 (`codex/issue-37-vla-partial-construction`, base commit `38027ab`). The implementation remains self-contained. Tests were written, run against the unchanged implementation, and committed before the fix: - `59fbcab`: deterministic moving-resource regressions for reserve, shrink, implicit growth, address-sensitive elements, move-only/copy-fallback lifetimes, allocation and construction failures, trivial storage, packed bool, and allocators without reallocate. - `90ed933`: single-allocation resource behavior, plus explicit capacity setup in existing copy-assignment and fill-resize tests that previously relied on non-trivial in-place growth. - `dc8e9c4`: production gate and documentation. The reallocation suite centralizes numeric scenario inputs and derives sizes, payloads, packed capacities, and lifetime expectations from them. Static assertions protect the relocation, failure-position, bit-pattern, and numeric-range requirements; implicit resize derives its target from the capacity obtained after append. Validation: - Unchanged #215: 14 of 22 new tests failed with GCC C++14 Debug and Clang C++17 Debug; 10 of 18 failed with GCC C++14 ReleaseEP. The eight control cases passed. The additional single-allocation regression also failed against an isolated copy of the unchanged header. - Fixed implementation: all 22 new tests pass with exceptions; all 18 applicable tests pass without exceptions. - All 10 VLA suites passed GCC/Clang × C++14/17 × Debug/ReleaseEP (8 configurations). Reallocation and exception-safety suites passed GCC/Clang × C++14/17/20/23 × Debug/Release/DebugEP/ReleaseEP (32 configurations). - Reallocation and exception-safety suites passed ASan/UBSan with leak detection in C++14 and C++17. - Constant refactor: the focused suite passed GCC/Clang × C++14/17 × Debug/ReleaseEP. Two alternate constant configurations passed with and without exceptions, and 12 invalid configurations were rejected by their intended static assertions. - Changed-code clang-format checks and `git diff --check` passed. Doxygen completed with the same 13 pre-existing warnings in type_traits_ext.hpp and unbounded_variant.hpp. Closes #213 after this stack is merged into the default branch.
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.
When an element constructor threw during a VLA bulk operation, already-constructed destination objects were not destroyed. Failed constructors and replacement-buffer relocation could also leak allocations. This change rolls back the constructed prefix and releases abandoned storage while keeping surviving containers usable.
Currently stacked on #214 for review; the base is
codex/issue-206-uninitialized-move. The VLA implementation is self-contained and has no dependency on the polyfill header.A VLA-local construction loop in the base tracks completed elements and destroys that prefix on failure, retaining allocator-aware construction/destruction, VLA's
move_if_noexceptpolicy, and trivial-type fast paths. The shared base owns storage cleanup, including when a derived constructor fails. Copy, move, default, and fill construction commit size only after success; replacement allocations are released if relocation fails. Assignment can retain already-assigned values or leave an empty destination after replacing its old buffer. Throwing move-only elements may leave source values changed, as documented.The regression suite injects first/middle/final failures into initializer-list/range/copy construction, unequal-allocator moves, reserve/shrink relocation, resize, and assignment. It checks exact object lifetimes, allocator hooks, allocation ownership and sizes, and container reuse after failure. It also covers failures from allocator construction hooks and source-range access, empty ranges, propagating allocators, throwing assignments, packed-bool range failures, and packed-bool bookkeeping after allocation failure. Incorrect unconditional
noexcepton copy-assignment helpers is removed so those failures can propagate.Validation:
variable_length_array.hppandcetl.hppavailable passed GCC/Clang C++14/17 with and without exceptions.memset(nullptr, ..., 0)) and null deallocation throughstd::pmr::polymorphic_allocator; both were independently reproduced against the unchanged Add C++17 uninitialized_move polyfill #214 header.type_traits_ext.hppandunbounded_variant.hppremain.git diff --checkpassed.Closes #37 after this stack is merged into the default branch.