Repository navigation
Add reverse iterators to VariableLengthArray (#65); move CI to toolshed ts26.4.5 - #244
Merged
Merged
Conversation
Add the reverse_iterator and const_reverse_iterator member types (std::reverse_iterator over iterator and const_iterator), and the rbegin(), rend(), crbegin() and crend() accessors, with const overloads of rbegin() and rend(), matching std::vector. This covers both ordinary elements and packed bool. All six accessors are noexcept, and like the forward iterators they are invalidated by reserve() and shrink_to_fit(). An array that has never allocated has an empty reverse range. For packed bool, the reverse iterators wrap the proxy iterators. Writing through them works, and because iterator converts to const_iterator, a reverse_iterator converts to a const_reverse_iterator and the two compare and subtract in either order. A new test_variable_length_array_reverse_iterators suite (int and bool, checked against std::vector) covers the member and return types, noexcept, traversal at byte-boundary sizes, base(), random access, the empty array, writing through the adaptor, mixed reverse iterators, a search from the back, and std::string elements through operator->. Closes #65. 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: 8176795137
ℹ️ 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".
Dereferencing a VLA<bool> iterator gives a temporary reference proxy, which std::swap cannot take. So algorithms that swap elements through std::iter_swap, such as std::reverse, std::rotate and std::sort, did not compile on bits, whether through the new reverse iterators or the forward ones. std::vector<bool> avoids this with a swap overload for its proxies. Add a hidden-friend swap(reference, reference) on the proxy, which ADL finds from std::iter_swap, and the static VariableLengthArray<bool>::swap(reference, reference) that std::vector<bool> also provides. Both exchange the two bits and are noexcept. A new test runs std::reverse over reverse and forward ranges, std::rotate, std::iter_swap and std::sort on bits against std::vector<bool>, and swaps proxies directly both ways. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
Preparation for moving CI to toolshed ts26.4.5. Its compilers and clang-format are newer than ts24.4.3's (GCC 15.2 for 13.3, Clang 22.1 for 19.1, clang-format 22.1 for 19.1, CMake 4.2 for 3.31), and every build runs the header format check. - clang-format 22 lays out four declarations differently from 19: three declarations whose constexpr sits behind a preprocessor guard, a defaulted template argument, and two enable_if return types. Wrap them in clang-format off/on, as the headers already do elsewhere, so both versions accept every header. The GCC 7 job stays on ts24.4.3 and clang-format 19. - type_traits_ext: detail::copy_list_initialize took a by-value To. For To = volatile int that is a volatile-qualified parameter, which C++20 deprecates, and Clang 22 reports it even in this unevaluated use (-Wdeprecated-volatile, an error under -Werror for anyone using is_convertible_without_narrowing with a volatile target). Take std::remove_cv_t<To> instead; top-level cv does not affect how the parameter is initialized. - example_07: drop `virtual` from the destructor of a final class whose base destructor is not virtual (-Wunnecessary-virtual-specifier, new in Clang 22). - test_variable_length_array_bool: the std::vector oracle used std::allocator<unsigned char> for std::vector<bool>. The standard requires the allocator's value_type to match, and GCC 15's libstdc++ enforces it in C++20 and later. Use a bool allocator for the oracle; VLA<bool> keeps its unsigned char allocator. Verified in ts26.4.3: the GCC and Clang preset workflows at C++14, 17, 20 and 23 (232 test runs each), the docs build and the online cache warmup; zero clang-format 19 or 22 violations in include/. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Run cache-warmup, verification-amd64, verification-arm64 and docs in ghcr.io/opencyphal/toolshed:ts26.4.5, the latest release (Ubuntu 26.04; GCC 15.2, Clang 22.1, clang-format 22.1, CMake 4.2). ts26.4.x no longer ships g++-7, which the gcc7-native toolchain pins. GCC 7 moves out of the arm64 matrix into a new verification-legacy-arm64 job that stays on ts24.4.3, the last image with it, so pinned old compilers have a home separate from the current-toolchain jobs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thirtytwobits
changed the title
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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
Adds [`std::vector::insert`](https://en.cppreference.com/w/cpp/container/vector/insert) to `cetl::VariableLengthArray`. All five overloads are included, for both the general and the packed-`bool` versions: ```cpp iterator insert(const_iterator pos, const T& value); iterator insert(const_iterator pos, T&& value); iterator insert(const_iterator pos, size_type count, const T& value); template <class InputIt> iterator insert(const_iterator pos, InputIt first, InputIt last); iterator insert(const_iterator pos, std::initializer_list<T> ilist); ``` These cover the range and initializer-list forms requested in the issue comments. Each returns an iterator to the first inserted element, or to `pos` if nothing was inserted. ## How it works New elements are constructed at the end of the array, then `std::rotate` moves them into place. - This needs only what the standard requires of `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. (The `bool` specialization rotates through the proxy `swap` added in #244.) - 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 | Situation | Result | | --- | --- | | Constructing a new element throws, or a single-pass source's read or advance throws | The new elements are destroyed and the array is unchanged, except that capacity may stay grown (as with `resize()`) | | Moving existing elements throws during the rotation | Every element stays alive and counted; their order is unspecified | | `insert(pos, v[i])` or `insert(pos, n, v[i])`, even when inserting reallocates | Correct: an aliased value is copied first | | Exceptions disabled; a sized insertion exceeds `max_size()` or the allocator declines | No effect; detect it with `size()` | | Exceptions disabled; a single-pass range doesn't fit | The prefix that fit is kept, 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 - **New `test_variable_length_array_insert.cpp`**, typed over `int` and `bool` and checked against `std::vector`: - every position in arrays of 0, 1, 7, 8, 9 and 17 elements (crossing packed-byte boundaries), for each overload and several counts, including random-access, forward, single-pass and initializer-list sources - mutable iterator positions, and empty insertions on a never-allocated array - geometric growth, and `max_size()` with and without exceptions - overload resolution (`insert(pos, 3, 4)` and `insert(pos, {5})`) - an aliased value, with and without reallocation - move-only elements, and elements that are constructible but not assignable from the source - no allocation when capacity suffices, declined growth, and narrowing conversions under `-Wconversion -Werror` - **Exception safety:** - failures while constructing, reading or advancing leave the array unchanged - rotation failures keep every element alive - allocation failure changes nothing - a `bool` conversion failure restores the bits - **Death tests:** for both new assertions. - **Fuzzer:** the `emplace_back` operation 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 - **libc++ bug:** libc++ 23's own `std::vector<bool>::insert` has undefined behaviour. It shifts a word by its full width in `__copy_backward_unaligned`, which UBSan reports. I reproduced it with plain `std::vector<bool>` and no CETL code. The fuzzer's reference model therefore rebuilds itself instead of calling `std::vector::insert`. - **CETL bug, filed separately as #245:** `push_back(v[0])` and `emplace_back(v[0])` store the wrong value when they have to grow the storage. `insert` doesn't have this problem. ## Validation - GCC 13, C++14: all 16 VLA test suites pass in Debug and DebugEP (`-fno-exceptions`) - GCC 7 (toolshed `ts24.4.3`): the insert and exception-safety suites pass with and without exceptions - toolshed `ts26.4.3`: the insert and exception-safety suites compile cleanly with GCC 15 (C++14, C++20) and Clang 22 (C++23, libc++) - Clang, C++20, ASan + UBSan (with pointer-overflow checking): the insert and exception-safety suites pass - GCC 13 and Clang, C++14/17/20/23, with and without exceptions: the insert, exception-safety, debug-assert and bool suites compile cleanly with `-Wall -Wextra -Werror -Wconversion -Wsign-conversion -Wfloat-equal -pedantic` - libFuzzer, default and embedded profiles: ctest passes, plus 90 s runs of about 850k–900k inputs each with no findings - Header formatting: clang-format 19.1.7 (`ts24.4.3`) and 22.1.2 (`ts26.4.3`) both report no changes; `git diff --check` is clean Closes #62. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- 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 reverse iterators to
cetl::VariableLengthArray, matchingstd::vector::rbeginandrend:Both the general and the packed-
boolversions get them. Forbool,std::reverse_iteratorwraps the existing proxy iterators: writing through them works, and since #242 madeiteratorconvert toconst_iterator, areverse_iteratorconverts to aconst_reverse_iteratorand the two compare and subtract in either order. Like the forward iterators, reverse iterators are invalidated byreserve()andshrink_to_fit(). An array that has never allocated has an empty reverse range.Tests
New
test_variable_length_array_reverse_iterators.cpp, typed overintandbooland checked againststd::vector:noexceptbool),distance,base(), and random access*rbegin() = …,std::fill) andstd::reverse_copyas an oraclereverse_iteratorandconst_reverse_iterator: conversion,==,!=,<,>and subtraction in both ordersstd::findstd::stringelements, includingoperator->through the adaptorI left out a "
const_reverse_iteratordoesn't convert toreverse_iterator" check. Before C++20, libstdc++'sstd::reverse_iteratorconverting constructor is unconstrained (the constraint came with LWG 3435), so the trait reportstruethere even forstd::vector. It would test the standard library rather than VLA.Validation
-fno-exceptions)-Wall -Wextra -Werror -Wconversion -Wsign-conversion -pedanticts24.4.3: the new suite compiles cleanly with GCC 7 (C++14), GCC 13 (C++20) and Clang 19 (C++20)clang-format19.1.7 fromtoolshed:ts24.4.3reports no changes;git diff --checkis cleanBit proxy swap (from review)
VLA<bool>'s proxyreferencenow has a hidden-friendswap(reference, reference), and the class has the staticswap(reference, reference)thatstd::vector<bool>provides. Without them,std::reverse,std::rotate,std::sortand other algorithms that swap throughstd::iter_swapdid 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.3tots26.4.5, the latest release (Ubuntu 26.04).cache-warmupts26.4.5verification-amd64ts26.4.5gcc-native-32,clang-native× C++14/17/20/23verification-arm64ts26.4.5gcc-native,clang-native× C++14/17/20verification-legacy-arm64(new)ts24.4.3gcc7-native× C++14docsts26.4.5ts26.4.xno longer shipsg++-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:optional.hpp,variant.hppandrtti.hppdifferently from 19, so they are wrapped inclang-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_initializedeclared a by-valuevolatileparameter forvolatiletargets. C++20 deprecates that, and Clang 22 errors on it under-Werror, which would also hit users. It now takesstd::remove_cv_t<To>.example_07: dropped avirtualdestructor in afinalclass (-Wunnecessary-virtual-specifier, new in Clang 22).test_variable_length_array_bool.cpp: thestd::vector<bool>oracle used anunsigned charallocator. The standard requiresbool, and GCC 15 enforces it from C++20.Checked in
ts26.4.3, which has the same toolchains asts26.4.5; later releases only added linters and Bazel. Results: thegcc-nativeandclang-nativepreset workflows at C++14/17/20/23 (232 test runs each), thedocsbuild, and the online cache warmup. I couldn't rungcc-native-32locally, so this PR's CI is its first run on the new image.Closes #65.
🤖 Generated with Claude Code