Repository navigation
Add erase() to VariableLengthArray (#79) - #242
Merged
Merged
Conversation
Add `erase(const_iterator pos)` and `erase(const_iterator first, const_iterator last)`, matching std::vector::erase, for both ordinary elements and packed bool. Later elements move toward the front, and the vacated tail is destroyed (or, for bool, the bit count is reduced). Capacity and max_size() are unchanged, iterators before the erased position stay valid, and the returned iterator points at the element that followed the removed range. An empty range has no effect, including on an array that has never allocated. The general erase is noexcept when value_type's move assignment and destructor are. If a move assignment throws, the size and every object's lifetime are unchanged, although values may have been moved from. Packed-bool erase cannot fail and is noexcept. std::vector<bool>::erase takes const_iterator, so the bool specialization's mutable iterator now converts implicitly to its const_iterator, as a standard container's iterator does. The bool size-update arithmetic shared with range assignment moves into a set_size_bits() helper. New debug assertions reject a non-dereferenceable position (CDE_vla_009, CDE_vla_011 for bool) and a range outside the array (CDE_vla_010, CDE_vla_012 for bool). Tests: a new test_variable_length_array_erase suite (int and bool, checked against std::vector) covers every position and every range across packed-byte boundaries, returned iterators, capacity, mutable and const iterators, empty ranges, erase-while-iterating, iterator validity, std::string and move-only elements, and noexcept. The exception-safety suite checks object lifetimes on success and a throwing move assignment, the debug-assert suite has death tests for the four new assertions, and the fuzzer's pop_back operation now also chooses erase(pos) or erase(first, last) from its spare opcode bits, without changing the input format. Closes #79. 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1af2700e0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
EraseEverythingKeepsCapacityAndMaxSize compared `actual.erase(...)` with `actual.end()` inside one EXPECT_EQ. The order in which those arguments are evaluated is unspecified, and GCC's 32-bit C++20 build evaluated end() first, so it compared against the end from before the erase. Store each erase result before comparing it, here and in the other tests that combined the two. Also return end() straight away when the array has no storage, instead of computing `data_ + (first - cbegin())` from null pointers. C++ defines that arithmetic, but end() already avoids it, and the early return keeps erase the same way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7c0b84af1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
The bool specialization's iterator converts to const_iterator, but its comparison and difference operators were members taking the same iterator type. Only a right-hand iterator could convert, so `bits.begin() != bits.cend()` and `bits.end() - bits.cbegin()` did not compile, although they work for std::vector. Make ==, !=, <, >, <=, >= and iterator difference hidden friends, so either operand can convert. A new typed test covers every mixed combination in both orders. In the fuzzer's erase path, erase an empty array as `erase(cbegin(), cend())` and check the result with `== end()`, so the harness does no arithmetic on a possibly null data pointer. This matches how it already avoids subtracting null pointers elsewhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
thirtytwobits
added a commit
that referenced
this pull request
Oct 19, 2026
…ed ts26.4.5 (#244) Adds reverse iterators to `cetl::VariableLengthArray`, matching [`std::vector::rbegin`](https://en.cppreference.com/w/cpp/container/vector/rbegin) and [`rend`](https://en.cppreference.com/w/cpp/container/vector/rend): ```cpp using reverse_iterator = std::reverse_iterator<iterator>; using const_reverse_iterator = std::reverse_iterator<const_iterator>; reverse_iterator rbegin() noexcept; const_reverse_iterator rbegin() const noexcept; reverse_iterator rend() noexcept; const_reverse_iterator rend() const noexcept; const_reverse_iterator crbegin() const noexcept; const_reverse_iterator crend() const noexcept; ``` Both the general and the packed-`bool` versions get them. For `bool`, `std::reverse_iterator` wraps the existing proxy iterators: writing through them works, and since #242 made `iterator` convert to `const_iterator`, a `reverse_iterator` converts to a `const_reverse_iterator` and the two compare and subtract in either order. Like the forward iterators, reverse iterators are invalidated by `reserve()` and `shrink_to_fit()`. An array that has never allocated has an empty reverse range. ## Tests New `test_variable_length_array_reverse_iterators.cpp`, typed over `int` and `bool` and checked against `std::vector`: - member types, return types of every accessor (mutable and const), and `noexcept` - reverse traversal at sizes 0, 1, 7, 8, 9 and 17 (packed-byte boundaries for `bool`), `distance`, `base()`, and random access - the empty range of a never-allocated array - writing through the adaptor (`*rbegin() = …`, `std::fill`) and `std::reverse_copy` as an oracle - mixed `reverse_iterator` and `const_reverse_iterator`: conversion, `==`, `!=`, `<`, `>` and subtraction in both orders - searching from the back with `std::find` - `std::string` elements, including `operator->` through the adaptor I left out a "`const_reverse_iterator` doesn't convert to `reverse_iterator`" check. Before C++20, libstdc++'s `std::reverse_iterator` converting constructor is unconstrained (the constraint came with LWG 3435), so the trait reports `true` there even for `std::vector`. It would test the standard library rather than VLA. ## Validation - GCC 13, C++14: all 15 VLA test suites pass in Debug and DebugEP (`-fno-exceptions`) - GCC 13, C++20, and Clang C++20 with ASan + UBSan: the new suite passes - GCC 13 and Clang, C++14/17/20/23, with and without exceptions: the new, compat and bool suites compile cleanly with `-Wall -Wextra -Werror -Wconversion -Wsign-conversion -pedantic` - toolshed `ts24.4.3`: the new suite compiles cleanly with GCC 7 (C++14), GCC 13 (C++20) and Clang 19 (C++20) - `clang-format` 19.1.7 from `toolshed:ts24.4.3` reports no changes; `git diff --check` is clean ## Bit proxy swap (from review) `VLA<bool>`'s proxy `reference` now has a hidden-friend `swap(reference, reference)`, and the class has the static `swap(reference, reference)` that `std::vector<bool>` provides. Without them, `std::reverse`, `std::rotate`, `std::sort` and other algorithms that swap through `std::iter_swap` did not compile on bits, with forward iterators as well as the new reverse ones. ## CI: toolshed ts26.4.5 CI moves from `toolshed:ts24.4.3` to `ts26.4.5`, the latest release (Ubuntu 26.04). | Job | Image | Matrix | | --- | --- | --- | | `cache-warmup` | `ts26.4.5` | — | | `verification-amd64` | `ts26.4.5` | `gcc-native-32`, `clang-native` × C++14/17/20/23 | | `verification-arm64` | `ts26.4.5` | `gcc-native`, `clang-native` × C++14/17/20 | | `verification-legacy-arm64` (new) | `ts24.4.3` | `gcc7-native` × C++14 | | `docs` | `ts26.4.5` | — | `ts26.4.x` no longer ships `g++-7`, so GCC 7 moves to its own job on the last image that has it. The new image upgrades GCC 13.3 to 15.2, Clang 19.1 to 22.1, clang-format 19.1 to 22.1, and CMake 3.31 to 4.2. That needed a few fixes, in a separate commit: - **clang-format:** every build runs the header format check. clang-format 22 lays out four declarations in `optional.hpp`, `variant.hpp` and `rtti.hpp` differently from 19, so they are wrapped in `clang-format off/on`. Every header now has zero violations under both 19 (the GCC 7 job) and 22. - **`type_traits_ext.hpp`:** `detail::copy_list_initialize` declared a by-value `volatile` parameter for `volatile` targets. C++20 deprecates that, and Clang 22 errors on it under `-Werror`, which would also hit users. It now takes `std::remove_cv_t<To>`. - **`example_07`:** dropped a `virtual` destructor in a `final` class (`-Wunnecessary-virtual-specifier`, new in Clang 22). - **`test_variable_length_array_bool.cpp`:** the `std::vector<bool>` oracle used an `unsigned char` allocator. The standard requires `bool`, and GCC 15 enforces it from C++20. Checked in `ts26.4.3`, which has the same toolchains as `ts26.4.5`; later releases only added linters and Bazel. Results: the `gcc-native` and `clang-native` preset workflows at C++14/17/20/23 (232 test runs each), the `docs` build, and the online cache warmup. I couldn't run `gcc-native-32` locally, so this PR's CI is its first run on the new image. Closes #65. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
thirtytwobits
added a commit
that referenced
this pull request
Oct 19, 2026
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>
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
1 participant
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
erase()tocetl::VariableLengthArray, matchingstd::vector::erase:Both the general and the packed-
boolversions:bool, they shorten the bit count)max_size(); iterators before the erased position stay validend()if the range reached itExceptions. The general
eraseisnoexceptwhenvalue_type's move assignment and destructor are. If a move assignment throws, the size and every object's lifetime are unchanged, but values may have been moved from. Packed-boolerase can't fail and isnoexcept.booliterators.std::vector<bool>::erasetakesconst_iterator, sobits.erase(bits.begin())needsiteratorto convert toconst_iterator. Theboolspecialization's proxy iterator had no such conversion, so this PR adds one, as standard containers provide. Aconst_iteratorstill doesn't convert toiterator.Debug assertions. Four new checks:
CDE_vla_009(general) andCDE_vla_011(bool): the position is not dereferenceable, e.g.end()CDE_vla_010(general) andCDE_vla_012(bool): the range is outside the array, reversed, or, forbool, from another arrayThe
boolsize update that range assignment already used moves into a smallset_size_bits()helper shared witherase.Tests
test_variable_length_array_erase.cpp, typed overintandbooland checked againststd::vector. It covers:[first, last)of a 17-element array, across packed-byte boundariespush_backafterward to check the bit bookkeepingstd::stringandstd::unique_ptrelements, and thenoexceptspecificationtest_variable_length_array_exception_safety.cpp: erase destroys only the vacated tail, and a throwing move assignment keeps the size and every lifetime.test_variable_length_array_debug_asserts.cpp: death tests for the four new assertions.pop_backoperation now also chooseserase(pos)orerase(first, last)from its spare opcode bits, so the input format is unchanged. It checks the returned position and capacity, and compares the result againststd::vectorafter every operation.Validation
-fno-exceptions), including the death teststs24.4.3): the erase, exception-safety and death-test suites pass with and without exceptions, with-Werror -Wconversion -Wsign-conversion-Wall -Wextra -Werror -Wconversion -Wsign-conversion -pedanticclang-format19.1.7 fromtoolshed:ts24.4.3reports no changes for the header, the new suite or the fuzzer;git diff --checkis clean. The new death tests follow the existing style oftest_variable_length_array_debug_asserts.cpp, which isn't clang-formatted.Closes #79.
🤖 Generated with Claude Code