Extensions for testing

[rolandreichweinbmw]

This PR extends several aspects of testing:

• Support for cross-compiling armhf, i386, powerpc, riscv64 and (extending) s390x, to run in CI checks via qemu
• Devcontainers for the aforementioned
• Fixes to source code to support all those architectures
• Extending run-tests.sh to run tests for all supported C++ versions
• Fixing exit code of run-tests.sh (always returned 0)
• Documentation of testing and generators

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds cross-architecture Dev Container Dockerfiles and VS Code devcontainer configurations for armhf, i386, riscv64 and s390x, plus per-architecture CMake toolchain files. Introduces .devcontainer/run-tests.sh for Docker+QEMU orchestration and new GitHub Actions workflows for armhf, i386, riscv64 and s390x CI. Adds documentation pages docs/generators.md and docs/testing.md. Adds include/etl/private/diagnostic_sign_conversion_push.h and wraps placement-new sites with diagnostic push/pop. Applies targeted source and test edits: explicit casts, midpoint divisor literal changes, bitset index types switched to size_t, exposes EXTRA_TESTING_FLAGS to CTest, renames a CMake test flag, and updates s390x devcontainer to create a non-root user.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Extensions for testing' is vague and generic, using non-descriptive language that does not clearly convey the changeset's main purpose despite the PR actually implementing cross-architecture testing infrastructure.	Consider using a more descriptive title such as 'Add cross-compilation testing support for armhf, i386, and riscv64' to better reflect the primary changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The PR description clearly outlines multiple testing infrastructure enhancements aligned with the changeset: cross-compilation support for armhf/i386/riscv64/s390x via QEMU, devcontainers, source code fixes, run-tests.sh improvements, and documentation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/run-tests.sh (1)

251-251: ⚠️ Potential issue | 🟡 Minor

Inconsistent compiler setting for clang builtin mem functions test.

This line is labelled as clang but uses gcc/g++ for the compilers, which appears to be a copy-paste error from the gcc variant above.

Proposed fix

-clang,No STL - Builtin mem functions ,.,cmake -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ -DNO_STL=ON  -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=OFF  -DETL_OPTIMISATION=$opt -DETL_CXX_STANDARD=$cxx_standard -DETL_ENABLE_SANITIZER=$sanitize -DETL_MESSAGES_ARE_NOT_VIRTUAL=OFF -DETL_USE_BUILTIN_MEM_FUNCTIONS=ON ..
+clang,No STL - Builtin mem functions ,.,cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DNO_STL=ON  -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=OFF  -DETL_OPTIMISATION=$opt -DETL_CXX_STANDARD=$cxx_standard -DETL_ENABLE_SANITIZER=$sanitize -DETL_MESSAGES_ARE_NOT_VIRTUAL=OFF -DETL_USE_BUILTIN_MEM_FUNCTIONS=ON ..

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` at line 251, The test entry labeled "clang,No STL -
Builtin mem functions" incorrectly sets CMAKE_C_COMPILER and CMAKE_CXX_COMPILER
to gcc/g++; update that test invocation to use clang/clang++ (replace
-DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ with -DCMAKE_C_COMPILER=clang
-DCMAKE_CXX_COMPILER=clang++ in the command string) so the label matches the
compilers used and the clang variant actually runs with clang/clang++; ensure no
other flags are changed and keep -DETL_USE_BUILTIN_MEM_FUNCTIONS=ON intact.

🧹 Nitpick comments (8)

include/etl/private/bitset_new.h (1)

1081-1082: Good fix for sign-conversion, but compile-time overloads remain inconsistent.

Changing element_index to size_t correctly addresses sign-conversion warnings for runtime extraction. However, the compile-time template overload at line 1189 still declares Msb_Element_Index as int, which is then passed to this function expecting size_t. This creates an implicit sign conversion.

Consider also updating line 1189 for consistency:

-      const int Msb_Element_Index = (Position + Length - 1) >> etl::log2<Bits_Per_Element>::value;
+      const size_t Msb_Element_Index = (Position + Length - 1) >> etl::log2<Bits_Per_Element>::value;

Similarly, line 1172 uses int for Element_Index where size_t would be more consistent with the pattern established here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@include/etl/private/bitset_new.h` around lines 1081 - 1082, The compile-time
template overloads are still using signed int template parameters
(Msb_Element_Index and Element_Index) while extract_from_multiple_elements now
takes size_t, causing implicit sign conversions; update the template parameter
types in the compile-time overloads (the templates that forward to
extract_from_multiple_elements and are declared with Msb_Element_Index and
Element_Index) from int to size_t so the types match, and ensure any related
constexpr helper signatures and calls use size_t consistently to eliminate the
mismatch.

.devcontainer/armhf/Dockerfile (1)

1-40: Consider adding a non-root user for improved security posture.

Static analysis flagged that the container runs as root. While this is common and acceptable for development/testing containers, adding a non-root user would align with container security best practices. This is optional for devcontainers used in CI/testing.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/armhf/Dockerfile around lines 1 - 40, Add a non-root user in
the Dockerfile by creating a new user/group (e.g., etl) and home directory
during the RUN steps, chowning the WORKDIR (/workspaces/etl) to that user,
setting ENV HOME appropriately, and then switching context with the USER
instruction before the CMD; update the existing RUN that installs packages to
also adduser/addgroup and chown the workspace so subsequent commands and the
default CMD run as the non-root user instead of root.

test/run-tests.sh (2)

202-206: Consider modernising the shell syntax.

Shellcheck flags the use of legacy backticks and missing quotes. Using $(...) is more readable and nestable.

Proposed fix

-  while read i ; do
-    CC=`echo $i | cut -d, -f1 | sed -e 's/ *$//'`
-    MSG=`echo $i | cut -d, -f2 | sed -e 's/ *$//'`
-    DIR=`echo $i | cut -d, -f3 | sed -e 's/ *$//'`
-    CMD=`echo $i | cut -d, -f4 | sed -e 's/ *$//'`
+  while read -r i ; do
+    CC=$(echo "$i" | cut -d, -f1 | sed -e 's/ *$//')
+    MSG=$(echo "$i" | cut -d, -f2 | sed -e 's/ *$//')
+    DIR=$(echo "$i" | cut -d, -f3 | sed -e 's/ *$//')
+    CMD=$(echo "$i" | cut -d, -f4 | sed -e 's/ *$//')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` around lines 202 - 206, The loop parsing CSV fields uses
legacy backticks and lacks proper quoting; update the parsing lines in the
while-read loop (the lines that set CC, MSG, DIR, CMD) to use POSIX command
substitution $(...) and properly quote variable expansions (e.g., "$(echo "$i" |
cut -d, -f1 | sed -e 's/ *$//')" etc.), or better yet split $i with IFS and read
to avoid external echo/cut/sed; ensure the while read i; do remains compatible
by quoting expansions when using $i and when assigning CC, MSG, DIR, CMD.

---

214-217: Add error handling for cd commands.

If cd $DIR fails, the script continues and may execute commands in an unexpected directory.

Proposed fix

-      OLD_DIR=`pwd`
-      cd $DIR
-      mkdir -p build-make || exit 1
-      cd build-make || exit 1
+      OLD_DIR=$(pwd)
+      cd "$DIR" || exit 1
+      mkdir -p build-make || exit 1
+      cd build-make || exit 1

And at line 238:

-      cd $OLD_DIR
+      cd "$OLD_DIR" || exit 1

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` around lines 214 - 217, The script does not handle failure
of directory changes (e.g., cd $DIR and cd build-make) so subsequent commands
may run in the wrong location; update the block around OLD_DIR, DIR and
build-make to check each cd return and exit with a non-zero status (or print an
error and exit) on failure — e.g., after cd $DIR and after cd build-make ensure
you abort if the cd fails (use conditional checks or append || { echo "error";
exit 1; }) so the script never continues in the wrong directory.

.devcontainer/i386/Dockerfile (1)

10-10: Minor: Missing space before line continuation.

There's a missing space between --no-install-recommends and the line continuation backslash.

Suggested fix

-    apt-get update && apt-get install -y --no-install-recommends\
+    apt-get update && apt-get install -y --no-install-recommends \

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/i386/Dockerfile at line 10, The apt-get install line has the
backslash directly adjacent to the token "--no-install-recommends", causing a
malformed line continuation; update the Dockerfile line containing "apt-get
update && apt-get install -y --no-install-recommends\" to insert a single space
before the trailing backslash so the line continuation is separated from the
option token.

.devcontainer/riscv64/Dockerfile (1)

10-10: Minor: Missing space before line continuation.

Same formatting issue as the i386 Dockerfile - missing space between --no-install-recommends and the backslash.

Suggested fix

-    apt-get update && apt-get install -y --no-install-recommends\
+    apt-get update && apt-get install -y --no-install-recommends \

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/riscv64/Dockerfile at line 10, The Dockerfile line with
"apt-get update && apt-get install -y --no-install-recommends\\" is missing a
space before the line-continuation backslash; update the line (in the
.devcontainer riscv64 Dockerfile where that apt-get install command appears) to
ensure there is a space between "--no-install-recommends" and the trailing
backslash so the shell line-continuation is correct and matches the i386
Dockerfile style.

.github/workflows/gcc-c++23-riscv64.yml (1)

46-46: Consider quoting the command substitution.

Actionlint flags the unquoted $(getconf _NPROCESSORS_ONLN). While safe here (returns a number), quoting is defensive practice.

Proposed fix

-        cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)
+        cmake --build . -- -j "$(getconf _NPROCESSORS_ONLN)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/gcc-c++23-riscv64.yml at line 46, The build invocation
uses an unquoted command substitution in the cmake build step: "cmake --build .
-- -j $(getconf _NPROCESSORS_ONLN)"; update this to quote the substitution
(e.g., change to use "$(getconf _NPROCESSORS_ONLN)") so the shell won’t perform
word-splitting or globbing on the result and Actionlint stops flagging the
unquoted substitution.

.github/workflows/gcc-c++23-s390x.yml (1)

23-24: Remove sudo from the apt-get commands in this container job.

When running in a container, the job executes as root by default. The sudo commands on lines 23–24 are unnecessary and should be removed for clarity.

💡 Proposed fix

-        sudo apt-get update
-        sudo apt-get install -y \
+        apt-get update
+        apt-get install -y \

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/gcc-c++23-s390x.yml around lines 23 - 24, Remove the
unnecessary sudo prefixes from the apt commands in the workflow: change "sudo
apt-get update" to "apt-get update" and "sudo apt-get install -y \" to "apt-get
install -y \" (preserve indentation and the line continuation backslash), since
container jobs run as root and sudo is redundant.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.devcontainer/run-tests.sh:
- Around line 46-60: The script branch for elif [ "$2" = "inside_container" ]
handles the container case but leaves other non-empty $2 values silently
succeeding; modify .devcontainer/run-tests.sh to add a final else branch after
that elif which prints a usage/error message (e.g., "Usage: ./run-tests.sh
[mode] — valid mode: inside_container") and exits with non-zero status (exit 1),
ensuring invalid values of $2 are handled explicitly; locate the conditional
that checks "$2" = "inside_container" and append the else { echo "..."; exit 1;
} block.

In @.github/workflows/gcc-c++23-armhf.yml:
- Around line 23-24: The workflow steps use "sudo apt-get update" and "sudo
apt-get install -y \" inside container: debian:trixie where sudo is not
available; remove the "sudo" prefix from the apt-get commands (e.g., change
"sudo apt-get update" and "sudo apt-get install -y \" to "apt-get update" and
"apt-get install -y \" ) in this file and apply the same change to the other
cross-architecture workflow files (gcc-c++23-armhf.yml, gcc-c++23-i386.yml,
gcc-c++23-riscv64.yml, gcc-c++23-s390x.yml) so package installs run directly as
root in the container.

In @.github/workflows/gcc-c++23-i386.yml:
- Around line 23-24: Remove the unnecessary "sudo" prefix from the two commands
currently written as "sudo apt-get update" and "sudo apt-get install -y \"; edit
the workflow (.github/workflows/gcc-c++23-i386.yml) so those lines call "apt-get
update" and "apt-get install -y ..." directly to match the root entrypoint usage
(leave the rest of the install arguments unchanged).

In @.github/workflows/gcc-c++23-riscv64.yml:
- Around line 23-24: Remove the unnecessary sudo prefixes from the apt-get
invocations—replace "sudo apt-get update" and "sudo apt-get install -y \" with
plain "apt-get update" and "apt-get install -y \" in the workflow(s) that use
the debian:trixie container (the four architecture workflows: armhf, i386,
riscv64, s390x), ensuring no other apt-get uses still include sudo so the job
runs as root inside the container.

In `@test/CMakeLists.txt`:
- Line 387: The run-tests.sh script exports the EXTRA_TESTING_FLAGS shell
variable but never passes it into CMake; update the cmake invocations inside the
heredoc (the cmake commands that configure the test build) to add
-DEXTRA_TESTING_FLAGS="${EXTRA_TESTING_FLAGS}" so the cached CMake variable
EXTRA_TESTING_FLAGS receives the value from the environment; ensure the added -D
flag is present for both configuration commands in the heredoc so behavior
matches the GitHub workflow usage.

In `@test/run-tests.sh`:
- Around line 183-189: The script currently exports EXTRA_TESTING_FLAGS as an
environment variable which has no effect because CMake reads EXTRA_TESTING_FLAGS
as a cache variable at configure time; change the logic so that when the `"$6" =
"v"` branch sets verbose, you build a cmake configure flag (e.g. set
`verbose_cmake_flag="-DEXTRA_TESTING_FLAGS=-v"`) and pass that flag into the
cmake configure commands inside the heredoc instead of exporting the env var;
update references to use `verbose_cmake_flag` in the cmake invocation(s) so
CMake receives the `-DEXTRA_TESTING_FLAGS=-v` option at configure time.

In `@test/test_histogram.cpp`:
- Line 206: The test uses unsigned subtraction in the CHECK_EQUAL assertion
causing underflow when i < 4; change the index computation to perform signed
subtraction by casting i to int first (i.e., replace static_cast<int>(i - 4)
with static_cast<int>(i) - 4) so the comparison between output1[i] and
histogram[...] uses a proper signed index; update the CHECK_EQUAL line that
references output1, histogram, and i accordingly.

---

Outside diff comments:
In `@test/run-tests.sh`:
- Line 251: The test entry labeled "clang,No STL - Builtin mem functions"
incorrectly sets CMAKE_C_COMPILER and CMAKE_CXX_COMPILER to gcc/g++; update that
test invocation to use clang/clang++ (replace -DCMAKE_C_COMPILER=gcc
-DCMAKE_CXX_COMPILER=g++ with -DCMAKE_C_COMPILER=clang
-DCMAKE_CXX_COMPILER=clang++ in the command string) so the label matches the
compilers used and the clang variant actually runs with clang/clang++; ensure no
other flags are changed and keep -DETL_USE_BUILTIN_MEM_FUNCTIONS=ON intact.

---

Nitpick comments:
In @.devcontainer/armhf/Dockerfile:
- Around line 1-40: Add a non-root user in the Dockerfile by creating a new
user/group (e.g., etl) and home directory during the RUN steps, chowning the
WORKDIR (/workspaces/etl) to that user, setting ENV HOME appropriately, and then
switching context with the USER instruction before the CMD; update the existing
RUN that installs packages to also adduser/addgroup and chown the workspace so
subsequent commands and the default CMD run as the non-root user instead of
root.

In @.devcontainer/i386/Dockerfile:
- Line 10: The apt-get install line has the backslash directly adjacent to the
token "--no-install-recommends", causing a malformed line continuation; update
the Dockerfile line containing "apt-get update && apt-get install -y
--no-install-recommends\" to insert a single space before the trailing backslash
so the line continuation is separated from the option token.

In @.devcontainer/riscv64/Dockerfile:
- Line 10: The Dockerfile line with "apt-get update && apt-get install -y
--no-install-recommends\\" is missing a space before the line-continuation
backslash; update the line (in the .devcontainer riscv64 Dockerfile where that
apt-get install command appears) to ensure there is a space between
"--no-install-recommends" and the trailing backslash so the shell
line-continuation is correct and matches the i386 Dockerfile style.

In @.github/workflows/gcc-c++23-riscv64.yml:
- Line 46: The build invocation uses an unquoted command substitution in the
cmake build step: "cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)"; update
this to quote the substitution (e.g., change to use "$(getconf
_NPROCESSORS_ONLN)") so the shell won’t perform word-splitting or globbing on
the result and Actionlint stops flagging the unquoted substitution.

In @.github/workflows/gcc-c++23-s390x.yml:
- Around line 23-24: Remove the unnecessary sudo prefixes from the apt commands
in the workflow: change "sudo apt-get update" to "apt-get update" and "sudo
apt-get install -y \" to "apt-get install -y \" (preserve indentation and the
line continuation backslash), since container jobs run as root and sudo is
redundant.

In `@include/etl/private/bitset_new.h`:
- Around line 1081-1082: The compile-time template overloads are still using
signed int template parameters (Msb_Element_Index and Element_Index) while
extract_from_multiple_elements now takes size_t, causing implicit sign
conversions; update the template parameter types in the compile-time overloads
(the templates that forward to extract_from_multiple_elements and are declared
with Msb_Element_Index and Element_Index) from int to size_t so the types match,
and ensure any related constexpr helper signatures and calls use size_t
consistently to eliminate the mismatch.

In `@test/run-tests.sh`:
- Around line 202-206: The loop parsing CSV fields uses legacy backticks and
lacks proper quoting; update the parsing lines in the while-read loop (the lines
that set CC, MSG, DIR, CMD) to use POSIX command substitution $(...) and
properly quote variable expansions (e.g., "$(echo "$i" | cut -d, -f1 | sed -e
's/ *$//')" etc.), or better yet split $i with IFS and read to avoid external
echo/cut/sed; ensure the while read i; do remains compatible by quoting
expansions when using $i and when assigning CC, MSG, DIR, CMD.
- Around line 214-217: The script does not handle failure of directory changes
(e.g., cd $DIR and cd build-make) so subsequent commands may run in the wrong
location; update the block around OLD_DIR, DIR and build-make to check each cd
return and exit with a non-zero status (or print an error and exit) on failure —
e.g., after cd $DIR and after cd build-make ensure you abort if the cd fails
(use conditional checks or append || { echo "error"; exit 1; }) so the script
never continues in the wrong directory.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 01189e0a-5938-4e90-a97f-e57668405783

📥 Commits

Reviewing files that changed from the base of the PR and between 912e22f and 8fc3e07.

📒 Files selected for processing (33)

• .devcontainer/armhf/Dockerfile
• .devcontainer/armhf/devcontainer.json
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/i386/Dockerfile
• .devcontainer/i386/devcontainer.json
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/Dockerfile
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/run-tests.sh
• .devcontainer/s390x/devcontainer.json
• .github/workflows/gcc-c++23-armhf.yml
• .github/workflows/gcc-c++23-i386.yml
• .github/workflows/gcc-c++23-riscv64.yml
• .github/workflows/gcc-c++23-s390x.yml
• docs/generators.md
• docs/testing.md
• include/etl/algorithm.h
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/ipool.h
• include/etl/numeric.h
• include/etl/private/bitset_new.h
• include/etl/private/diagnostic_sign_conversion_push.h
• include/etl/vector.h
• test/CMakeLists.txt
• test/run-tests.sh
• test/test_bit_stream.cpp
• test/test_bit_stream_writer_big_endian.cpp
• test/test_bit_stream_writer_little_endian.cpp
• test/test_byte_stream.cpp
• test/test_format.cpp
• test/test_histogram.cpp
• test/test_set.cpp

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (4)

include/etl/private/bitset_new.h (1)

1136-1137: Align compile-time extract_from_multiple_elements calls with size_t indices.

Lines 1189 and 1172 declare Msb_Element_Index and Element_Index as int, but the function signature at line 1081 expects size_t for the element-index parameter. This causes implicit sign-conversion when passed at lines 1194 and elsewhere. Update these declarations to use size_t to match the runtime path (line 1153) and avoid potential diagnostics.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@include/etl/private/bitset_new.h` around lines 1136 - 1137, The compile-time
callers of extract_from_multiple_elements use int-typed indices (Element_Index
and Msb_Element_Index) causing implicit signed-to-unsigned conversions; change
the declarations of Element_Index and Msb_Element_Index to size_t so they match
the extract_from_multiple_elements signature (which expects a size_t
element-index) and the runtime path that already uses size_t. Update the two
declarations that compute indexes via shifts against
etl::log2<Bits_Per_Element>::value to use size_t instead of int, ensuring all
calls to extract_from_multiple_elements pass size_t indices.

.devcontainer/armhf/Dockerfile (1)

3-3: Pin the base image by digest for reproducible CI.

Line [3] uses a floating tag (debian:trixie), which can silently change over time and destabilise cross-arch test runs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/armhf/Dockerfile at line 3, Replace the floating base image
reference "FROM debian:trixie" with a pinned digest form (e.g., "FROM
debian@sha256:...") so the build is reproducible; fetch the correct official
Debian trixie armhf image digest and update the Dockerfile line, then verify by
pulling the image and rebuilding CI to ensure the digest matches the intended
architecture.

.devcontainer/i386/Dockerfile (1)

3-3: Pin the base image digest to avoid drift.

Line [3] currently tracks a mutable tag (debian:trixie), which can make CI results non-reproducible over time.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/i386/Dockerfile at line 3, The Dockerfile currently uses a
mutable base image tag "FROM debian:trixie"; replace it with a digest-pinned
reference (the RepoDigest form like debian@sha256:<digest>) to ensure
reproducible builds: update the FROM line to the specific sha256 digest for the
desired trixie image (obtain the digest by pulling or inspecting the
debian:trixie image and using its RepoDigest) and commit that pinned reference
in place of "debian:trixie".

docs/testing.md (1)

70-72: Tighten sentence punctuation in list lead-ins for readability.

The lead-in sentences before numbered steps read slightly awkwardly; adding a small comma/flow polish would improve scanability.

Also applies to: 140-151

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/testing.md` around lines 70 - 72, The lead-in sentences are missing
commas and read awkwardly; update the phrases to improve flow—e.g., change "For
every selected C++ standard the script loops over a built-in list of
configurations (STL / No STL / Force C++03 / Non-virtual messages / …) for each
selected compiler. For every combination it:" to include commas like "For every
selected C++ standard, the script loops over a built-in list of configurations
(...) for each selected compiler. For every combination, it:" and apply the same
comma/flow fixes to the other affected block referenced ("Also applies to:
140-151") so the lead-ins before numbered steps consistently include the
necessary commas and read naturally.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.devcontainer/armhf/Dockerfile:
- Around line 39-40: Current Dockerfile leaves the container running as root
(ends with CMD ["/bin/bash"]); add a non-root runtime user by creating a
user/group (with a stable/non-zero UID/GID), set its home and shell, chown
necessary workspace/config directories to that user, and add a USER <username>
instruction before CMD so the container runs unprivileged; ensure the username
and any created home directory are referenced in those steps (e.g., in the
Dockerfile's user creation and chown lines) so runtime processes no longer run
as root.

In @.devcontainer/i386/Dockerfile:
- Around line 39-40: The Dockerfile currently ends with CMD ["/bin/bash"] and
lacks a USER instruction, so update the final image to run as a non-root user by
creating a dedicated user and group (e.g., appuser), setting a home directory,
adjusting ownership of any workspace/build directories, and adding USER appuser
before the final CMD; ensure the created user's UID/GID are non-zero and any
files the container needs at runtime are chowned to that user so the container
no longer runs as root.

In @.devcontainer/run-tests.sh:
- Around line 43-44: The docker run invocation in run-tests.sh uses a relative
bind mount "-v .:/workspaces/etl" which can break when the script is executed
from another directory; update the docker run command in run-tests.sh to use an
absolute, quoted host path (e.g. use $PWD or $(pwd -P)) for the bind mount
(replace "-v .:/workspaces/etl" with a quoted "-v \"$PWD\":/workspaces/etl") so
the container always mounts the correct project directory.

In @.github/workflows/gcc-c++23-s390x.yml:
- Around line 42-43: The CMake invocation currently sets
-DETL_FORCE_TEST_CPP03=OFF but the build expects
-DETL_FORCE_TEST_CPP03_IMPLEMENTATION; update all workflow CMake command lines
(the cmake invocation that includes -DETL_CXX_STANDARD=23) to replace
-DETL_FORCE_TEST_CPP03=OFF with -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=OFF so the
CMake cache variable referenced in test/CMakeLists.txt
(ETL_FORCE_TEST_CPP03_IMPLEMENTATION) is actually set; apply this change across
the indicated C++23 workflow files (e.g., the cmake line shown in
gcc-c++23-s390x.yml).

In `@docs/testing.md`:
- Line 75: Replace occurrences of the hyphenated term "sub-directory" with the
single-word "subdirectory" to ensure consistent technical prose; update both
instances referenced (the text containing "sub-directory" near Line 75 and the
other at Line 233) so the document uses "subdirectory" everywhere.

In `@include/etl/algorithm.h`:
- Line 561: The implementation of etl::fill uses a narrowing static_cast when
assigning to *first (the line with static_cast<typename
etl::iterator_traits<TIterator>::value_type>(value)), which is stricter than
fill_n and ranges::fill_fn and breaks valid code where *iterator = value is
allowed but constructing value_type(value) is not; change the assignment to
perform a direct assignment (*first = value) consistent with fill_n and
ranges::fill_fn (and std::fill) so the function relies on assignment semantics
rather than requiring static_cast constructibility.

In `@test/run-tests.sh`:
- Around line 183-189: The script sets verbose and verbose_cmake_flag from
argument 6 but always calls ctest with -V; change the ctest invocation to
respect the verbose setting by adding a ctest_verbose_flag (or reuse verbose)
and only include -V when verbose="On" (so use verbose/verbose_cmake_flag to
build the ctest command instead of unconditionally using -V), ensuring the
existing verbose_cmake_flag behavior is preserved.

---

Nitpick comments:
In @.devcontainer/armhf/Dockerfile:
- Line 3: Replace the floating base image reference "FROM debian:trixie" with a
pinned digest form (e.g., "FROM debian@sha256:...") so the build is
reproducible; fetch the correct official Debian trixie armhf image digest and
update the Dockerfile line, then verify by pulling the image and rebuilding CI
to ensure the digest matches the intended architecture.

In @.devcontainer/i386/Dockerfile:
- Line 3: The Dockerfile currently uses a mutable base image tag "FROM
debian:trixie"; replace it with a digest-pinned reference (the RepoDigest form
like debian@sha256:<digest>) to ensure reproducible builds: update the FROM line
to the specific sha256 digest for the desired trixie image (obtain the digest by
pulling or inspecting the debian:trixie image and using its RepoDigest) and
commit that pinned reference in place of "debian:trixie".

In `@docs/testing.md`:
- Around line 70-72: The lead-in sentences are missing commas and read
awkwardly; update the phrases to improve flow—e.g., change "For every selected
C++ standard the script loops over a built-in list of configurations (STL / No
STL / Force C++03 / Non-virtual messages / …) for each selected compiler. For
every combination it:" to include commas like "For every selected C++ standard,
the script loops over a built-in list of configurations (...) for each selected
compiler. For every combination, it:" and apply the same comma/flow fixes to the
other affected block referenced ("Also applies to: 140-151") so the lead-ins
before numbered steps consistently include the necessary commas and read
naturally.

In `@include/etl/private/bitset_new.h`:
- Around line 1136-1137: The compile-time callers of
extract_from_multiple_elements use int-typed indices (Element_Index and
Msb_Element_Index) causing implicit signed-to-unsigned conversions; change the
declarations of Element_Index and Msb_Element_Index to size_t so they match the
extract_from_multiple_elements signature (which expects a size_t element-index)
and the runtime path that already uses size_t. Update the two declarations that
compute indexes via shifts against etl::log2<Bits_Per_Element>::value to use
size_t instead of int, ensuring all calls to extract_from_multiple_elements pass
size_t indices.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 70818d0b-5aff-43fd-adfa-66d1a19cfe18

📥 Commits

Reviewing files that changed from the base of the PR and between 8fc3e07 and 7a40024.

📒 Files selected for processing (34)

• .devcontainer/armhf/Dockerfile
• .devcontainer/armhf/devcontainer.json
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/i386/Dockerfile
• .devcontainer/i386/devcontainer.json
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/Dockerfile
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/run-tests.sh
• .devcontainer/s390x/Dockerfile
• .devcontainer/s390x/devcontainer.json
• .github/workflows/gcc-c++23-armhf.yml
• .github/workflows/gcc-c++23-i386.yml
• .github/workflows/gcc-c++23-riscv64.yml
• .github/workflows/gcc-c++23-s390x.yml
• docs/generators.md
• docs/testing.md
• include/etl/algorithm.h
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/ipool.h
• include/etl/numeric.h
• include/etl/private/bitset_new.h
• include/etl/private/diagnostic_sign_conversion_push.h
• include/etl/vector.h
• test/CMakeLists.txt
• test/run-tests.sh
• test/test_bit_stream.cpp
• test/test_bit_stream_writer_big_endian.cpp
• test/test_bit_stream_writer_little_endian.cpp
• test/test_byte_stream.cpp
• test/test_format.cpp
• test/test_histogram.cpp
• test/test_set.cpp

✅ Files skipped from review due to trivial changes (17)

• .devcontainer/s390x/devcontainer.json
• .devcontainer/s390x/Dockerfile
• test/test_bit_stream_writer_big_endian.cpp
• test/test_byte_stream.cpp
• include/etl/bip_buffer_spsc_atomic.h
• test/test_bit_stream.cpp
• .devcontainer/i386/devcontainer.json
• test/test_set.cpp
• test/test_histogram.cpp
• include/etl/private/diagnostic_sign_conversion_push.h
• test/test_bit_stream_writer_little_endian.cpp
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/armhf/devcontainer.json
• include/etl/vector.h
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/i386/toolchain-i386.cmake

🚧 Files skipped from review as they are similar to previous changes (3)

• test/CMakeLists.txt
• include/etl/numeric.h
• include/etl/ipool.h

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

.github/workflows/clang-c++23.yml (1)

62-62: ⚠️ Potential issue | 🟠 Major

Inconsistent CMake variable name in Force C++03 jobs.

Lines 62, 91, 157, and 179 use -DETL_FORCE_TEST_CPP03=ON, but test/CMakeLists.txt defines ETL_FORCE_TEST_CPP03_IMPLEMENTATION. CMake will silently ignore the undefined variable, so the Force C++03 behaviour won't be enabled.

🐛 Proposed fix

# Lines 62, 91, 157, 179 - change all occurrences:
-        cmake ... -DETL_FORCE_TEST_CPP03=ON ...
+        cmake ... -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON ...

Also applies to: 91-91, 157-157, 179-179

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/clang-c++23.yml at line 62, The workflow uses the wrong
CMake variable name: replace every occurrence of -DETL_FORCE_TEST_CPP03=ON in
the CMake invocation(s) with -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON so the
flag defined in test/CMakeLists.txt is actually set; update the CMake command(s)
(the lines invoking cmake -D BUILD_TESTS=ON ... -DETL_FORCE_TEST_CPP03=ON ./) to
use -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON for the Force C++03 jobs.

.github/workflows/gcc-c++23.yml (1)

70-70: ⚠️ Potential issue | 🟠 Major

Inconsistent CMake variable name in Force C++03 jobs.

Lines 70 and 92 use -DETL_FORCE_TEST_CPP03=ON, but test/CMakeLists.txt defines and checks ETL_FORCE_TEST_CPP03_IMPLEMENTATION. CMake will silently ignore the undefined variable, so the Force C++03 behaviour won't actually be enabled.

🐛 Proposed fix

# Line 70
-        cmake -DBUILD_TESTS=ON -DNO_STL=OFF -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03=ON -DETL_CXX_STANDARD=23 ./
+        cmake -DBUILD_TESTS=ON -DNO_STL=OFF -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON -DETL_CXX_STANDARD=23 ./

# Line 92
-        cmake -DBUILD_TESTS=ON -DNO_STL=ON -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03=ON -DETL_CXX_STANDARD=23 ./
+        cmake -DBUILD_TESTS=ON -DNO_STL=ON -DETL_USE_TYPE_TRAITS_BUILTINS=OFF -DETL_USER_DEFINED_TYPE_TRAITS=OFF -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON -DETL_CXX_STANDARD=23 ./

Also applies to: 92-92

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/gcc-c++23.yml at line 70, The CMake invocation is passing
the wrong variable name; replace the flag -DETL_FORCE_TEST_CPP03=ON with
-DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON in the cmake command invocations so the
test/CMakeLists.txt check sees the defined variable; update both cmake command
occurrences that currently pass ETL_FORCE_TEST_CPP03 to use
ETL_FORCE_TEST_CPP03_IMPLEMENTATION instead.

🧹 Nitpick comments (3)

test/run-tests.sh (2)

217-217: Add error handling for cd commands.

Shellcheck SC2164 warns that cd can fail silently. If the directory doesn't exist or is inaccessible, the script will continue in the wrong directory, potentially causing confusing failures.

♻️ Proposed fix

-      cd $DIR
+      cd "$DIR" || exit 1
...
-      cd $OLD_DIR
+      cd "$OLD_DIR" || exit 1

Also applies to: 240-240

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` at line 217, The script uses bare cd commands (e.g., cd
$DIR and other cd invocations) which can fail silently; update each cd to check
for errors and bail out or handle them (for example replace cd $DIR with cd
"$DIR" || { echo "Failed to cd to $DIR"; exit 1; } or similar), ensuring you
quote variables and apply the same pattern to the other cd occurrences mentioned
so the script stops or reports a clear error if directory change fails.

---

204-208: Consider modernising shell syntax.

Shellcheck flags several style issues: use $() instead of backticks (SC2006), add -r to read (SC2162), and quote variable expansions (SC2086). These are minor but improve robustness.

♻️ Proposed fix for one example

-  while read i ; do
-    CC=`echo $i | cut -d, -f1 | sed -e 's/ *$//'`
+  while read -r i ; do
+    CC=$(echo "$i" | cut -d, -f1 | sed -e 's/ *$//')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` around lines 204 - 208, The loop that parses CSV fields
uses backticks, unquoted expansions, and plain read; update the while read to
use read -r (e.g. while read -r i) and replace all backtick usages with $(...)
(affecting the CC, MSG, DIR, CMD assignments), and quote variable expansions
(use "${i}" when passing into echo/cut/sed or, preferably, split once with
IFS=',' and read -r CC MSG DIR CMD to avoid multiple cut calls) while preserving
the trimming logic for each variable.

.github/workflows/gcc-c++23-s390x.yml (1)

46-46: Consider quoting the command substitution.

Shellcheck SC2046 warns about word splitting with $(getconf _NPROCESSORS_ONLN). While _NPROCESSORS_ONLN reliably returns a single integer, quoting is a defensive best practice.

♻️ Proposed fix

-        cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)
+        cmake --build . -- -j "$(getconf _NPROCESSORS_ONLN)"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/gcc-c++23-s390x.yml at line 46, Update the cmake build
step that currently uses an unquoted command substitution (the line containing
"cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)") by wrapping the command
substitution in double quotes so the result is quoted; this prevents potential
word-splitting issues flagged by ShellCheck SC2046 while keeping the same
semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.devcontainer/run-tests.sh:
- Around line 57-59: Replace the direct emulator invocation (the line running
qemu-$QEMU_ARCH-static ./etl_tests) with a CTest-driven invocation so CMake’s
test integration and EXTRA_TESTING_FLAGS are respected; specifically, stop
calling etl_tests directly and run ctest (e.g., ctest --output-on-failure) from
the build/test directory so CTest picks up CMAKE_CROSSCOMPILING_EMULATOR from
the toolchain file and executes the etl_tests target through the test harness.

In `@docs/generators.md`:
- Around line 145-146: Update the docs/generators.md statement that claims
generator.yml runs scripts/generator_test.py: change it to accurately describe
the actual workflow (generator.yml) behavior by noting it executes the
repository generator (generate.bat / equivalent generate script) and then runs
git diff --exit-code to verify no uncommitted changes remain; reference the
workflow file name generator.yml, the incorrect mention
scripts/generator_test.py, the actual generate.bat invocation, and the git diff
--exit-code check so contributors understand the real verification step.

---

Outside diff comments:
In @.github/workflows/clang-c++23.yml:
- Line 62: The workflow uses the wrong CMake variable name: replace every
occurrence of -DETL_FORCE_TEST_CPP03=ON in the CMake invocation(s) with
-DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON so the flag defined in
test/CMakeLists.txt is actually set; update the CMake command(s) (the lines
invoking cmake -D BUILD_TESTS=ON ... -DETL_FORCE_TEST_CPP03=ON ./) to use
-DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON for the Force C++03 jobs.

In @.github/workflows/gcc-c++23.yml:
- Line 70: The CMake invocation is passing the wrong variable name; replace the
flag -DETL_FORCE_TEST_CPP03=ON with -DETL_FORCE_TEST_CPP03_IMPLEMENTATION=ON in
the cmake command invocations so the test/CMakeLists.txt check sees the defined
variable; update both cmake command occurrences that currently pass
ETL_FORCE_TEST_CPP03 to use ETL_FORCE_TEST_CPP03_IMPLEMENTATION instead.

---

Nitpick comments:
In @.github/workflows/gcc-c++23-s390x.yml:
- Line 46: Update the cmake build step that currently uses an unquoted command
substitution (the line containing "cmake --build . -- -j $(getconf
_NPROCESSORS_ONLN)") by wrapping the command substitution in double quotes so
the result is quoted; this prevents potential word-splitting issues flagged by
ShellCheck SC2046 while keeping the same semantics.

In `@test/run-tests.sh`:
- Line 217: The script uses bare cd commands (e.g., cd $DIR and other cd
invocations) which can fail silently; update each cd to check for errors and
bail out or handle them (for example replace cd $DIR with cd "$DIR" || { echo
"Failed to cd to $DIR"; exit 1; } or similar), ensuring you quote variables and
apply the same pattern to the other cd occurrences mentioned so the script stops
or reports a clear error if directory change fails.
- Around line 204-208: The loop that parses CSV fields uses backticks, unquoted
expansions, and plain read; update the while read to use read -r (e.g. while
read -r i) and replace all backtick usages with $(...) (affecting the CC, MSG,
DIR, CMD assignments), and quote variable expansions (use "${i}" when passing
into echo/cut/sed or, preferably, split once with IFS=',' and read -r CC MSG DIR
CMD to avoid multiple cut calls) while preserving the trimming logic for each
variable.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 74fd80fd-a3e2-43f8-adf1-70b42c3f5900

📥 Commits

Reviewing files that changed from the base of the PR and between 7a40024 and dbbfd94.

📒 Files selected for processing (36)

• .devcontainer/armhf/Dockerfile
• .devcontainer/armhf/devcontainer.json
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/i386/Dockerfile
• .devcontainer/i386/devcontainer.json
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/Dockerfile
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/run-tests.sh
• .devcontainer/s390x/Dockerfile
• .devcontainer/s390x/devcontainer.json
• .github/workflows/clang-c++23.yml
• .github/workflows/gcc-c++23-armhf.yml
• .github/workflows/gcc-c++23-i386.yml
• .github/workflows/gcc-c++23-riscv64.yml
• .github/workflows/gcc-c++23-s390x.yml
• .github/workflows/gcc-c++23.yml
• docs/generators.md
• docs/testing.md
• include/etl/algorithm.h
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/ipool.h
• include/etl/numeric.h
• include/etl/private/bitset_new.h
• include/etl/private/diagnostic_sign_conversion_push.h
• include/etl/vector.h
• test/CMakeLists.txt
• test/run-tests.sh
• test/test_bit_stream.cpp
• test/test_bit_stream_writer_big_endian.cpp
• test/test_bit_stream_writer_little_endian.cpp
• test/test_byte_stream.cpp
• test/test_format.cpp
• test/test_histogram.cpp
• test/test_set.cpp

✅ Files skipped from review due to trivial changes (21)

• include/etl/ipool.h
• .devcontainer/s390x/devcontainer.json
• test/test_set.cpp
• test/test_bit_stream.cpp
• test/test_format.cpp
• test/test_bit_stream_writer_big_endian.cpp
• .devcontainer/armhf/toolchain-armhf.cmake
• include/etl/bip_buffer_spsc_atomic.h
• test/test_bit_stream_writer_little_endian.cpp
• include/etl/private/diagnostic_sign_conversion_push.h
• .devcontainer/i386/devcontainer.json
• .devcontainer/armhf/devcontainer.json
• test/test_histogram.cpp
• .devcontainer/riscv64/devcontainer.json
• include/etl/algorithm.h
• .devcontainer/i386/Dockerfile
• .devcontainer/armhf/Dockerfile
• .devcontainer/riscv64/Dockerfile
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/toolchain-riscv64.cmake
• test/test_byte_stream.cpp

🚧 Files skipped from review as they are similar to previous changes (3)

• .devcontainer/s390x/Dockerfile
• include/etl/numeric.h
• include/etl/vector.h

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

test/run-tests.sh (1)

206-210: Consider modernising shell syntax for robustness.

Shellcheck flags several style/safety items: using legacy backticks instead of $(...) and unquoted variables that could cause word splitting with unusual directory names.

♻️ Suggested improvements

-  while read i ; do
-    CC=`echo $i | cut -d, -f1 | sed -e 's/ *$//'`
-    MSG=`echo $i | cut -d, -f2 | sed -e 's/ *$//'`
-    DIR=`echo $i | cut -d, -f3 | sed -e 's/ *$//'`
-    CMD=`echo $i | cut -d, -f4 | sed -e 's/ *$//'`
+  while read -r i ; do
+    CC=$(echo "$i" | cut -d, -f1 | sed -e 's/ *$//')
+    MSG=$(echo "$i" | cut -d, -f2 | sed -e 's/ *$//')
+    DIR=$(echo "$i" | cut -d, -f3 | sed -e 's/ *$//')
+    CMD=$(echo "$i" | cut -d, -f4 | sed -e 's/ *$//')

-      OLD_DIR=`pwd`
-      cd $DIR
+      OLD_DIR=$(pwd)
+      cd "$DIR"

-      cd $OLD_DIR
+      cd "$OLD_DIR"

Also applies to: 218-219, 240-240

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` around lines 206 - 210, In the while read loop that parses
CSV into CC, MSG, DIR, CMD, replace legacy backticks with modern command
substitution $(...) (e.g., use $(echo ...) and $(cut ...)), switch the loop to
read -r and set IFS=, to avoid word-splitting, and ensure all variable
expansions are quoted (use "$CC", "$MSG", "$DIR", "$CMD"); apply the same
changes to the other occurrences noted (the blocks handling the same parsing at
the other ranges) so trimming/whitespace handling is preserved while preventing
globbing and splitting issues.

.devcontainer/run-tests.sh (2)

21-37: QEMU_ARCH variable is unused.

The QEMU_ARCH variable is set for each architecture but never used. The script now correctly uses ctest --output-on-failure which relies on CMAKE_CROSSCOMPILING_EMULATOR from the toolchain file. Consider removing the unused variable assignments to avoid confusion.

♻️ Suggested fix

 if [ "$1" = "armhf" ] ; then
   ARCH=armhf
-  QEMU_ARCH=arm
 elif [ "$1" = "i386" ] ; then
   ARCH=i386
-  QEMU_ARCH=i386
 elif [ "$1" = "riscv64" ] ; then
   ARCH=riscv64
-  QEMU_ARCH=riscv64
 elif [ "$1" = "s390x" ] ; then
   ARCH=s390x
-  QEMU_ARCH=s390x
 else

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/run-tests.sh around lines 21 - 37, The script sets a QEMU_ARCH
variable in the architecture detection block (branches that assign QEMU_ARCH
alongside ARCH) but never uses it; remove all QEMU_ARCH assignments from the
if/elif/else branch (keep the ARCH assignments and the Unsupported architecture
handling/usage call) so the script only sets ARCH and does not define the unused
QEMU_ARCH symbol, cleaning up the variables in the architecture detection logic.

---

54-54: Separate declaration and assignment to avoid masking errors.

If nproc fails, the error code is masked by the export statement. This makes debugging harder if the command fails unexpectedly.

♻️ Suggested fix

-  export CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)
+  CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)
+  export CMAKE_BUILD_PARALLEL_LEVEL

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/run-tests.sh at line 54, Split the combined export/assignment
so the call to nproc is executed and verified before exporting: assign the
result of `nproc` to the variable `CMAKE_BUILD_PARALLEL_LEVEL`, check for
failure (or fall back to a safe default) if `nproc` fails, then run `export
CMAKE_BUILD_PARALLEL_LEVEL`; this ensures `nproc` errors aren’t masked by the
`export` statement.

.github/workflows/gcc-c++23-riscv64.yml (1)

46-46: Consider quoting command substitution.

Shellcheck flags that $(getconf _NPROCESSORS_ONLN) should be quoted to prevent word splitting. While the output is always a number, quoting is a defensive best practice.

-        cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)
+        cmake --build . -- -j "$(getconf _NPROCESSORS_ONLN)"

This applies to all four cross-architecture workflows (armhf, i386, riscv64, s390x).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/gcc-c++23-riscv64.yml at line 46, The shell command using
command substitution in the build step (the line with cmake --build . -- -j
$(getconf _NPROCESSORS_ONLN)) should quote the substitution to prevent
word-splitting; update it to use "$(getconf _NPROCESSORS_ONLN)" in that workflow
file and make the same change in the other cross-architecture workflow files
(armhf, i386, riscv64, s390x) so all occurrences of getconf _NPROCESSORS_ONLN
are quoted.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/testing.md`:
- Around line 198-207: Update the docs to accurately reflect that the test
runner uses CTest rather than invoking the binary directly: change the sentence
that says it runs `qemu-<qemu_arch>-static ./etl_tests` to state that
`.devcontainer/run-tests.sh` runs `ctest --output-on-failure`, which relies on
the toolchain file (`.devcontainer/<arch>/toolchain-<arch>.cmake`) setting
CMAKE_CROSSCOMPILING_EMULATOR so the `etl_tests` binary is executed under QEMU
transparently; reference the `.devcontainer/run-tests.sh`, `ctest
--output-on-failure`, `CMAKE_CROSSCOMPILING_EMULATOR`, and the
`toolchain-<arch>.cmake` names in the updated text.

---

Nitpick comments:
In @.devcontainer/run-tests.sh:
- Around line 21-37: The script sets a QEMU_ARCH variable in the architecture
detection block (branches that assign QEMU_ARCH alongside ARCH) but never uses
it; remove all QEMU_ARCH assignments from the if/elif/else branch (keep the ARCH
assignments and the Unsupported architecture handling/usage call) so the script
only sets ARCH and does not define the unused QEMU_ARCH symbol, cleaning up the
variables in the architecture detection logic.
- Line 54: Split the combined export/assignment so the call to nproc is executed
and verified before exporting: assign the result of `nproc` to the variable
`CMAKE_BUILD_PARALLEL_LEVEL`, check for failure (or fall back to a safe default)
if `nproc` fails, then run `export CMAKE_BUILD_PARALLEL_LEVEL`; this ensures
`nproc` errors aren’t masked by the `export` statement.

In @.github/workflows/gcc-c++23-riscv64.yml:
- Line 46: The shell command using command substitution in the build step (the
line with cmake --build . -- -j $(getconf _NPROCESSORS_ONLN)) should quote the
substitution to prevent word-splitting; update it to use "$(getconf
_NPROCESSORS_ONLN)" in that workflow file and make the same change in the other
cross-architecture workflow files (armhf, i386, riscv64, s390x) so all
occurrences of getconf _NPROCESSORS_ONLN are quoted.

In `@test/run-tests.sh`:
- Around line 206-210: In the while read loop that parses CSV into CC, MSG, DIR,
CMD, replace legacy backticks with modern command substitution $(...) (e.g., use
$(echo ...) and $(cut ...)), switch the loop to read -r and set IFS=, to avoid
word-splitting, and ensure all variable expansions are quoted (use "$CC",
"$MSG", "$DIR", "$CMD"); apply the same changes to the other occurrences noted
(the blocks handling the same parsing at the other ranges) so
trimming/whitespace handling is preserved while preventing globbing and
splitting issues.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d54dbd11-7ca0-41b6-baf6-65b47ce867ad

📥 Commits

Reviewing files that changed from the base of the PR and between dbbfd94 and 0bd865d.

📒 Files selected for processing (36)

• .devcontainer/armhf/Dockerfile
• .devcontainer/armhf/devcontainer.json
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/i386/Dockerfile
• .devcontainer/i386/devcontainer.json
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/Dockerfile
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/run-tests.sh
• .devcontainer/s390x/Dockerfile
• .devcontainer/s390x/devcontainer.json
• .github/workflows/clang-c++23.yml
• .github/workflows/gcc-c++23-armhf.yml
• .github/workflows/gcc-c++23-i386.yml
• .github/workflows/gcc-c++23-riscv64.yml
• .github/workflows/gcc-c++23-s390x.yml
• .github/workflows/gcc-c++23.yml
• docs/generators.md
• docs/testing.md
• include/etl/algorithm.h
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/ipool.h
• include/etl/numeric.h
• include/etl/private/bitset_new.h
• include/etl/private/diagnostic_sign_conversion_push.h
• include/etl/vector.h
• test/CMakeLists.txt
• test/run-tests.sh
• test/test_bit_stream.cpp
• test/test_bit_stream_writer_big_endian.cpp
• test/test_bit_stream_writer_little_endian.cpp
• test/test_byte_stream.cpp
• test/test_format.cpp
• test/test_histogram.cpp
• test/test_set.cpp

✅ Files skipped from review due to trivial changes (21)

• .devcontainer/s390x/devcontainer.json
• test/CMakeLists.txt
• include/etl/ipool.h
• test/test_bit_stream.cpp
• test/test_byte_stream.cpp
• test/test_histogram.cpp
• test/test_format.cpp
• .github/workflows/gcc-c++23.yml
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/private/diagnostic_sign_conversion_push.h
• test/test_bit_stream_writer_little_endian.cpp
• .devcontainer/i386/devcontainer.json
• include/etl/algorithm.h
• .devcontainer/armhf/devcontainer.json
• .devcontainer/riscv64/Dockerfile
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/armhf/Dockerfile
• include/etl/vector.h
• .devcontainer/i386/toolchain-i386.cmake
• test/test_bit_stream_writer_big_endian.cpp
• .devcontainer/i386/Dockerfile

🚧 Files skipped from review as they are similar to previous changes (7)

• .github/workflows/clang-c++23.yml
• .devcontainer/s390x/Dockerfile
• test/test_set.cpp
• include/etl/private/bitset_new.h
• include/etl/numeric.h
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake

[coderabbitai]

🧹 Nitpick comments (2)

.devcontainer/run-tests.sh (1)

50-50: Consider separating declaration and assignment.

Shellcheck SC2155 warns that combining export with command substitution can mask non-zero exit codes from nproc. While unlikely to fail in practice, separating the assignment would surface any issues.

♻️ Optional: separate declaration and assignment

-  export CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)
+  CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)
+  export CMAKE_BUILD_PARALLEL_LEVEL

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.devcontainer/run-tests.sh at line 50, The export combines declaration with
command substitution (export CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)), which can
mask non-zero exit codes (SC2155); change it to first declare/export the
variable name (export CMAKE_BUILD_PARALLEL_LEVEL) and then assign the result of
nproc to CMAKE_BUILD_PARALLEL_LEVEL with a separate command
(CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)), optionally checking the exit status of
nproc and handling errors before continuing.

test/run-tests.sh (1)

206-210: Consider modernising shell syntax for robustness.

The heredoc parsing uses legacy backticks and unquoted variables. While functional, Shellcheck flags these patterns (SC2006, SC2086, SC2162). This is low priority as the existing code uses these conventions, but quoting $i and using $(...) would improve robustness against filenames/values with spaces or special characters.

♻️ Optional: modernise shell syntax

-for cxx_standard in $cxx_standards ; do
-  while read i ; do
-    CC=`echo $i | cut -d, -f1 | sed -e 's/ *$//'`
-    MSG=`echo $i | cut -d, -f2 | sed -e 's/ *$//'`
-    DIR=`echo $i | cut -d, -f3 | sed -e 's/ *$//'`
-    CMD=`echo $i | cut -d, -f4 | sed -e 's/ *$//'`
+for cxx_standard in $cxx_standards ; do
+  while read -r i ; do
+    CC=$(echo "$i" | cut -d, -f1 | sed -e 's/ *$//')
+    MSG=$(echo "$i" | cut -d, -f2 | sed -e 's/ *$//')
+    DIR=$(echo "$i" | cut -d, -f3 | sed -e 's/ *$//')
+    CMD=$(echo "$i" | cut -d, -f4 | sed -e 's/ *$//')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/run-tests.sh` around lines 206 - 210, The loop parsing a CSV line
currently uses backticks and unquoted $i to populate CC, MSG, DIR, CMD; replace
legacy backticks with $(...) and ensure the input is quoted to avoid
word-splitting (e.g. use quoted "$i"), or better: use a safe read with IFS=','
and read -r to split into variables (read -r CC MSG DIR CMD <<< "$i" or read -r
-d '' ...), updating the assignments that set CC, MSG, DIR, CMD so they use
$(...) and quoted expansions to handle spaces/special characters robustly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.devcontainer/run-tests.sh:
- Line 50: The export combines declaration with command substitution (export
CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)), which can mask non-zero exit codes
(SC2155); change it to first declare/export the variable name (export
CMAKE_BUILD_PARALLEL_LEVEL) and then assign the result of nproc to
CMAKE_BUILD_PARALLEL_LEVEL with a separate command
(CMAKE_BUILD_PARALLEL_LEVEL=$(nproc)), optionally checking the exit status of
nproc and handling errors before continuing.

In `@test/run-tests.sh`:
- Around line 206-210: The loop parsing a CSV line currently uses backticks and
unquoted $i to populate CC, MSG, DIR, CMD; replace legacy backticks with $(...)
and ensure the input is quoted to avoid word-splitting (e.g. use quoted "$i"),
or better: use a safe read with IFS=',' and read -r to split into variables
(read -r CC MSG DIR CMD <<< "$i" or read -r -d '' ...), updating the assignments
that set CC, MSG, DIR, CMD so they use $(...) and quoted expansions to handle
spaces/special characters robustly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1ca2acba-8c52-4cce-b178-ad28110bc5b5

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd865d and 536e24f.

📒 Files selected for processing (36)

• .devcontainer/armhf/Dockerfile
• .devcontainer/armhf/devcontainer.json
• .devcontainer/armhf/toolchain-armhf.cmake
• .devcontainer/i386/Dockerfile
• .devcontainer/i386/devcontainer.json
• .devcontainer/i386/toolchain-i386.cmake
• .devcontainer/riscv64/Dockerfile
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/riscv64/toolchain-riscv64.cmake
• .devcontainer/run-tests.sh
• .devcontainer/s390x/Dockerfile
• .devcontainer/s390x/devcontainer.json
• .github/workflows/clang-c++23.yml
• .github/workflows/gcc-c++23-armhf.yml
• .github/workflows/gcc-c++23-i386.yml
• .github/workflows/gcc-c++23-riscv64.yml
• .github/workflows/gcc-c++23-s390x.yml
• .github/workflows/gcc-c++23.yml
• docs/generators.md
• docs/testing.md
• include/etl/algorithm.h
• include/etl/bip_buffer_spsc_atomic.h
• include/etl/ipool.h
• include/etl/numeric.h
• include/etl/private/bitset_new.h
• include/etl/private/diagnostic_sign_conversion_push.h
• include/etl/vector.h
• test/CMakeLists.txt
• test/run-tests.sh
• test/test_bit_stream.cpp
• test/test_bit_stream_writer_big_endian.cpp
• test/test_bit_stream_writer_little_endian.cpp
• test/test_byte_stream.cpp
• test/test_format.cpp
• test/test_histogram.cpp
• test/test_set.cpp

✅ Files skipped from review due to trivial changes (26)

• .devcontainer/s390x/devcontainer.json
• include/etl/bip_buffer_spsc_atomic.h
• test/CMakeLists.txt
• test/test_bit_stream_writer_little_endian.cpp
• test/test_bit_stream.cpp
• test/test_set.cpp
• test/test_bit_stream_writer_big_endian.cpp
• include/etl/private/diagnostic_sign_conversion_push.h
• test/test_byte_stream.cpp
• test/test_histogram.cpp
• .devcontainer/armhf/toolchain-armhf.cmake
• .github/workflows/gcc-c++23.yml
• .devcontainer/i386/devcontainer.json
• .devcontainer/riscv64/devcontainer.json
• .devcontainer/armhf/devcontainer.json
• include/etl/private/bitset_new.h
• .github/workflows/gcc-c++23-armhf.yml
• .devcontainer/i386/Dockerfile
• .github/workflows/gcc-c++23-riscv64.yml
• .devcontainer/armhf/Dockerfile
• .github/workflows/gcc-c++23-i386.yml
• .devcontainer/riscv64/Dockerfile
• .github/workflows/gcc-c++23-s390x.yml
• .devcontainer/i386/toolchain-i386.cmake
• include/etl/algorithm.h
• .devcontainer/riscv64/toolchain-riscv64.cmake

🚧 Files skipped from review as they are similar to previous changes (3)

• test/test_format.cpp
• .github/workflows/clang-c++23.yml
• include/etl/vector.h

[christophruethingbmw]

Looks good to me overall and really improves testing on non-x64 plaforms.

[christophruethingbmw]

As far as I can see below, you have created a non-root user in the image, is there a reason we still run it with root here (assuming the remoteUser option here specifies the user to use when connecting to the running container)?

[rolandreichweinbmw]

I took over the originally contributed devcontainer.json by @tzijnge . Not sure about the consequences of changing remoteUser now in the respective environments. E.g. in github workspaces: Do they need root permissions, e.g. to install via apt?