Repository navigation
Add insert() to VariableLengthArray (#62) - #246
Merged
Merged
Conversation
Add std::vector's five insert overloads to both the general and the packed-bool specializations: one copy, one moved value, count copies, an iterator range, and an initializer list. Each returns an iterator to the first inserted element, or to pos if nothing was inserted. New elements are constructed at the end of the array and std::rotate moves them into place. That needs only what the standard requires of insert (MoveAssignable and Swappable elements), and a range's elements need only be constructible from *first, not assignable from it. Capacity grows geometrically, as for push_back(), so repeated insertion stays amortized. Forward ranges are measured and reserved once; single-pass ranges are appended one element at a time. Guarantees: - If constructing a new element, or reading or advancing a single-pass source, throws, the new elements are destroyed and the array is unchanged except, possibly, capacity (as for resize()). - If moving existing elements throws during the rotation, every element stays alive and counted, in unspecified order. - insert(pos, value) and insert(pos, count, value) accept a value that is an element of the array, even when inserting reallocates: the value is copied first when it aliases (std::less address check). - With exceptions disabled, a sized insertion that exceeds max_size() or that the allocator declines has no effect. A single-pass range keeps the prefix that fit, as assign(first, last) does. - New debug assertions reject positions outside the array: CDE_vla_013, and CDE_vla_014 for bool, which also rejects another array's iterator. Tests: a new test_variable_length_array_insert suite (int and bool, checked against std::vector) covers every position of several array sizes across packed-byte boundaries with each overload and several counts, mutable iterators, empty insertions on a never-allocated array, geometric growth, max_size() with and without exceptions, overload resolution, an aliased value with and without reallocation, move-only and construct-only elements, no allocation within capacity, declined growth, and narrowing conversions under -Wconversion. The exception-safety suite covers construction, single-pass read and advance failures, rotation failures, allocation failure and bool conversion failures. Death tests cover both new assertions. The fuzzer's emplace_back operation now also chooses among the insert overloads from its spare opcode bits (input format unchanged) and checks each returned position. Its std::vector model now rebuilds itself instead of calling std::vector::insert, because libc++ 23's vector<bool>::insert hits undefined behaviour (a shift by the full word width in __copy_backward_unaligned) that UBSan reports, independent of CETL. Closes #62. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The insert tests and the fuzzer computed `x.insert(...) - x.begin()` and `x.insert(...) == x.begin() + n` in one expression. The order in which an expression's operands are evaluated is unspecified, and insert() may reallocate. GCC 15's 32-bit C++17 build evaluated std::vector's begin() first, then subtracted iterators into two different buffers, so verification-amd64 (gcc-native-32, cpp-17) failed AllOverloadsMatchVector against a garbage oracle. The same mistake was fixed in the erase tests in #242. Keep each returned iterator in a variable and compare it in a separate statement, in the tests and in the fuzzer. No library change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Adds
std::vector::inserttocetl::VariableLengthArray. All five overloads are included, for both the general and the packed-boolversions:These cover the range and initializer-list forms requested in the issue comments. Each returns an iterator to the first inserted element, or to
posif nothing was inserted.How it works
New elements are constructed at the end of the array, then
std::rotatemoves them into place.insert: elements must be MoveAssignable and Swappable. Elements are only constructed from a range's*first, never assigned from it, so a type that is constructible but not assignable from the source works. (Theboolspecialization rotates through the proxyswapadded in Add reverse iterators to VariableLengthArray (#65); move CI to toolshed ts26.4.5 #244.)push_back(), so repeated insertion stays amortized. Forward ranges are measured and reserved once; single-pass ranges are appended one element at a time.Guarantees
resize())insert(pos, v[i])orinsert(pos, n, v[i]), even when inserting reallocatesmax_size()or the allocator declinessize()assign(first, last)doesNew debug assertions reject positions outside the array:
CDE_vla_013, andCDE_vla_014forbool, which also rejects another array's iterator.Tests
test_variable_length_array_insert.cpp, typed overintandbooland checked againststd::vector:max_size()with and without exceptionsinsert(pos, 3, 4)andinsert(pos, {5}))-Wconversion -Werrorboolconversion failure restores the bitsemplace_backoperation now also chooses among the insert overloads from its spare opcode bits, so the input format is unchanged, and checks each returned position.Found along the way
std::vector<bool>::inserthas undefined behaviour. It shifts a word by its full width in__copy_backward_unaligned, which UBSan reports. I reproduced it with plainstd::vector<bool>and no CETL code. The fuzzer's reference model therefore rebuilds itself instead of callingstd::vector::insert.push_back(v[0])andemplace_back(v[0])store the wrong value when they have to grow the storage.insertdoesn't have this problem.Validation
-fno-exceptions)ts24.4.3): the insert and exception-safety suites pass with and without exceptionsts26.4.3: the insert and exception-safety suites compile cleanly with GCC 15 (C++14, C++20) and Clang 22 (C++23, libc++)-Wall -Wextra -Werror -Wconversion -Wsign-conversion -Wfloat-equal -pedanticts24.4.3) and 22.1.2 (ts26.4.3) both report no changes;git diff --checkis cleanCloses #62.
🤖 Generated with Claude Code