Debug assert #1175
Debug assert #1175
Conversation
WalkthroughIntroduces a comprehensive set of debug assertion macros in error_handler.h and a debug-only exception toggle in platform.h (ETL_DEBUG_USING_EXCEPTIONS / ETL_DEBUG_NOT_USING_EXCEPTIONS). Applies ETL_DEBUG_ASSERT-based runtime bounds checks to etl::array (operator[], insert, erase) and compile-time SIZE>0 checks for front()/back(). Replaces debug-guarded ETL_ASSERT checks in etl::expected with ETL_DEBUG_ASSERT. Updates tests and test profile to enable ETL_DEBUG and ETL_DEBUG_THROW_EXCEPTIONS and adds out-of-range tests for array operations. No public API signatures changed. Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.✨ Finishing Touches
🧪 Generate 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/test_array.cpp (1)
145-154: Same guard for the const variant, and make the container const in this test.Use the same conditional as above; also make the array const so this truly exercises the const operator[].
- Data data = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }; + const Data data = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }; @@ - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); + // Only active when debug + exceptions route is enabled for ETL_DEBUG_ASSERT +#if ETL_IS_DEBUG_BUILD && ETL_USING_EXCEPTIONS + CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); +#endifinclude/etl/array.h (1)
452-460: Replace TODOs with targeted debug asserts to prevent UB on iterator/index misuse.These operations will otherwise happily accept end() or inverted ranges and produce invalid moves. Suggested minimal checks:
iterator insert(const_iterator position, parameter_t value) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(position >= cbegin() && position < cend(), ETL_ERROR(array_out_of_range)); iterator p = to_iterator(position); etl::move_backward(p, end() - 1, end()); *p = value; return p; } @@ iterator insert(const_iterator position, TIterator first, const TIterator last) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(position >= cbegin() && position < cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(etl::distance(first, last) >= 0, ETL_ERROR(array_out_of_range)); // ensure forward (or empty) range iterator p = to_iterator(position); iterator result(p); size_t source_size = etl::distance(first, last); size_t destination_space = etl::distance(position, cend()); ... return result; } @@ iterator erase(const_iterator position) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(position >= cbegin() && position < cend(), ETL_ERROR(array_out_of_range)); iterator p = to_iterator(position); etl::move(p + 1, end(), p); return p; } @@ iterator erase(const_iterator first, const_iterator last) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(first >= cbegin() && first <= cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(last >= cbegin() && last <= cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(first <= last, ETL_ERROR(array_out_of_range)); iterator p = to_iterator(first); etl::move(last, cend(), p); return p; } @@ iterator erase(const_iterator position, parameter_t value) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(position >= cbegin() && position < cend(), ETL_ERROR(array_out_of_range)); iterator p = to_iterator(position); etl::move(p + 1, end(), p); back() = value; return p; } @@ iterator erase(const_iterator first, const_iterator last, parameter_t value) { - //TODO: Add relevant debug asserts + ETL_DEBUG_ASSERT(first >= cbegin() && first <= cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(last >= cbegin() && last <= cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(first <= last, ETL_ERROR(array_out_of_range)); iterator p = to_iterator(first); p = etl::move(last, cend(), p); etl::fill(p, end(), value); return to_iterator(first); }If you’d prefer not to depend on etl::distance sign for generic iterators, you can instead document that the iterators must form a non-negative range and only assert this in debug for random-access iterators.
Happy to wire these up across deque/array_view operator[] too for consistency in a follow-up.
Also applies to: 483-504, 521-529, 548-555, 572-582, 600-611
🧹 Nitpick comments (3)
include/etl/error_handler.h (1)
367-375: Decouple debug-assert throwing from global exceptions and make the macro single-statement safe.Nice addition. Two small tweaks will make it more flexible and robust:
- Add a separate switch (as mentioned in the PR text) to control throwing for debug asserts without tying it to ETL_USING_EXCEPTIONS.
- Wrap the macro in do { } while (0) to be statement-safe.
Apply:
-#if ETL_IS_DEBUG_BUILD - #if ETL_USING_EXCEPTIONS - #define ETL_DEBUG_ASSERT(b, e) {if (!(b)) ETL_UNLIKELY {throw((e));}} // If the condition fails, throws an exception. - #else - #define ETL_DEBUG_ASSERT(b, e) assert((b)) // If the condition fails, asserts. - #endif -#else - #define ETL_DEBUG_ASSERT(b, e) // Does nothing. -#endif +#ifndef ETL_DEBUG_USING_EXCEPTIONS +#define ETL_DEBUG_USING_EXCEPTIONS ETL_USING_EXCEPTIONS +#endif + +#if ETL_IS_DEBUG_BUILD + #if ETL_DEBUG_USING_EXCEPTIONS + #define ETL_DEBUG_ASSERT(b, e) do { if (!(b)) ETL_UNLIKELY { throw((e)); } } while (0) + #else + #define ETL_DEBUG_ASSERT(b, e) do { assert((b)); } while (0) + #endif +#else + #define ETL_DEBUG_ASSERT(b, e) /* ETL_DO_NOTHING */ +#endifinclude/etl/expected.h (1)
678-679: Good consolidation to ETL_DEBUG_ASSERT at all value-access points.This keeps debug-time behaviour consistent and centralised. Looks good.
Optionally mirror the same check in value() overloads for parity with operator* / operator->. For example (outside the changed hunk):
ETL_CONSTEXPR14 value_type& value()& { ETL_DEBUG_ASSERT(has_value(), ETL_ERROR(expected_invalid)); return etl::get<Value_Type>(storage); }Also applies to: 688-689, 698-699, 708-709, 719-720, 729-730
include/etl/array.h (1)
146-151: Bounds-check via ETL_DEBUG_ASSERT is a good addition; please confirm constexpr impact.
- LGTM on the check itself.
- One thing to confirm: in debug+exceptions builds, the const operator[] is ETL_CONSTEXPR14 and now lexically contains a throw in the dead branch. This is usually fine in C++14 as long as the branch isn’t taken in constant evaluation, but please ensure it compiles across your supported compilers for the constexpr tests.
If needed (optional), you can skip the check in constant-evaluated contexts where available:
#if ETL_USING_CPP20 if (!std::is_constant_evaluated()) { ETL_DEBUG_ASSERT(i < SIZE, ETL_ERROR(array_out_of_range)); } #else ETL_DEBUG_ASSERT(i < SIZE, ETL_ERROR(array_out_of_range)); #endifAlso applies to: 159-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
include/etl/array.h(12 hunks)include/etl/error_handler.h(1 hunks)include/etl/expected.h(6 hunks)test/test_array.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/test_array.cpp (1)
include/etl/array.h (3)
data(217-220)data(749-752)array_out_of_range(72-80)
include/etl/array.h (2)
include/etl/array_view.h (3)
operator(480-483)operator(489-492)operator(637-641)include/etl/deque.h (2)
operator(825-831)operator(837-843)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - No STL - Force C++03
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - No STL
🔇 Additional comments (2)
include/etl/error_handler.h (1)
367-375: No action required: flags already aliased
ETL_DEBUG and ETL_THROW_EXCEPTIONS are mapped to ETL_IS_DEBUG_BUILD and ETL_USING_EXCEPTIONS in include/etl/platform.h, so the tests and the ETL_DEBUG_ASSERT macro already use a consistent flag scheme.Likely an incorrect or invalid review comment.
include/etl/array.h (1)
173-176: Compile-time guards for front/back are sensible.The ETL_STATIC_ASSERTs make misuse on empty arrays a compile-time error for the primary template. Nice one.
Also applies to: 182-187, 196-199, 205-210
| //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined | ||
| CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); | ||
| } |
There was a problem hiding this comment.
Gate the out-of-bounds [] checks behind the same flags used by ETL_DEBUG_ASSERT.
Unconditionally requiring a throw will fail in release or non-exception builds. Guard the checks so they only run when ETL_IS_DEBUG_BUILD && ETL_USING_EXCEPTIONS (or ETL_DEBUG_USING_EXCEPTIONS if adopted).
- //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined
- CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range);
+ // Only active when debug + exceptions route is enabled for ETL_DEBUG_ASSERT
+#if ETL_IS_DEBUG_BUILD && ETL_USING_EXCEPTIONS
+ CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range);
+#endif🤖 Prompt for AI Agents
In test/test_array.cpp around lines 138 to 140, the test unconditionally expects
an exception from an out-of-bounds operator[] which will fail in release or
non-exception builds; wrap the CHECK_THROW block with a preprocessor guard so it
only runs when both debug and exceptions are enabled (e.g. #if
defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) ||
defined(ETL_DEBUG_USING_EXCEPTIONS))) and close the #endif after the
CHECK_THROW; ensure the guarded code preserves the existing CHECK_THROW
invocation and adds no additional side effects.
There was a problem hiding this comment.
I don't think this applies because there are many tests that that rely on exceptions and debug build already. Basically most tests already rely on these build options.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/etl/array.h (1)
482-504: Also assert the source range is valid to avoid negative distance underflow.If last precedes first, etl::distance(first, last) can be negative and will underflow when assigned to size_t, skewing the logic and risking copy_s misuse. Add a debug check for a non-negative distance (or first <= last for random access).
iterator insert(const_iterator position, TIterator first, const TIterator last) { - ETL_DEBUG_ASSERT(cbegin() <= position && position < cend(), ETL_ERROR(array_out_of_range)); + ETL_DEBUG_ASSERT(cbegin() <= position && position < cend(), ETL_ERROR(array_out_of_range)); + // Ensure a sane source range. + ETL_DEBUG_ASSERT(etl::distance(first, last) >= 0, ETL_ERROR(array_out_of_range));
♻️ Duplicate comments (1)
test/test_array.cpp (1)
138-139: Gate debug-only CHECK_THROWs behind build flags (will fail in release/no-exception builds).These throws should only be asserted when the ETL_DEBUG_ASSERT path raises exceptions; otherwise, release or no-exception configs will break. This mirrors the earlier review note for operator[] and should be applied consistently across insert/erase tests too.
Apply guards like this:
@@ - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); + // Only active when debug + exceptions are enabled for ETL_DEBUG_ASSERT +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); +#endif @@ - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); + // Only active when debug + exceptions are enabled for ETL_DEBUG_ASSERT +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + CHECK_THROW({ int d = data[data.size()]; (void)d; }, etl::array_out_of_range); +#endif @@ - // Insert out of range - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ result = data.insert_at(data.size(), 99); }, etl::array_out_of_range); + // Insert out of range +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + CHECK_THROW({ result = data.insert_at(data.size(), 99); }, etl::array_out_of_range); +#endif @@ - // Insert out of range - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ result = data.insert_at(data.size(), &source2[0], &source2[13]); }, etl::array_out_of_range); + // Insert out of range +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + CHECK_THROW({ result = data.insert_at(data.size(), &source2[0], &source2[13]); }, etl::array_out_of_range); +#endif @@ - // Erase out of range - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - CHECK_THROW({ result = data.erase_at(data.size()); }, etl::array_out_of_range); + // Erase out of range +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + CHECK_THROW({ result = data.erase_at(data.size()); }, etl::array_out_of_range); +#endif @@ - //ETL_DEBUG and ETL_THROW_EXCEPTIONS are defined - // Erase indexes are equal - CHECK_THROW({ result = data.erase_range(5, 5, 99); }, etl::array_out_of_range); - - // Erase out of range - CHECK_THROW({ result = data.erase_range(5, data.size() + 1, 99); }, etl::array_out_of_range); + // Debug-only range validation +#if defined(ETL_IS_DEBUG_BUILD) && (defined(ETL_USING_EXCEPTIONS) || defined(ETL_DEBUG_USING_EXCEPTIONS)) + // Erase indexes are equal + CHECK_THROW({ result = data.erase_range(5, 5, 99); }, etl::array_out_of_range); + // Erase out of range + CHECK_THROW({ result = data.erase_range(5, data.size() + 1, 99); }, etl::array_out_of_range); +#endifIf the macro names differ in your config, please tweak the guard to match error_handler.h. I can adjust once you confirm the canonical flags.
Also applies to: 152-153, 453-456, 507-510, 565-567, 623-629
🧹 Nitpick comments (1)
include/etl/array.h (1)
548-555: Erase range preconditions are strict; reflect this in the comments.Code requires first < last (not equal). Consider noting “first must be strictly less than last” in both erase-range overload docs to avoid surprises.
Also applies to: 601-611
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
include/etl/array.h(12 hunks)test/test_array.cpp(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_array.cpp (1)
include/etl/array.h (1)
array_out_of_range(72-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 OSX - STL (macos-13)
- GitHub Check: Clang C++20 OSX - No STL - Force C++03 (macos-13)
- GitHub Check: Clang C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL - Force C++03 (macos-13)
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Windows - No STL - Force C++03
- GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
🔇 Additional comments (3)
include/etl/array.h (3)
148-151: Bounds-checking for operator[] looks good; please confirm constexpr builds stay green.ETL_DEBUG_ASSERT inside ETL_CONSTEXPR14 operator[] should be fine, but please confirm the cpp14 constexpr tests pass in debug + exceptions builds.
Also applies to: 159-164
173-175: Nice safety: static_assert on front()/back() for SIZE > 0.This prevents accidental ODR use with zero-sized arrays at compile time.
Also applies to: 184-186, 196-199, 207-210
452-462: Position bounds checks via ETL_DEBUG_ASSERT are appropriate.Precondition coverage reads well and matches the tests.
Also applies to: 521-529, 572-582
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/etl/platform.h (1)
254-263: Gate debug-exception toggle on ETL_DEBUG and surface an introspection flagNice addition. Two small tweaks to avoid surprises and aid tests/introspection:
- Only enable the debug-exception path when ETL_DEBUG is also set; otherwise ETL_DEBUG_USING_EXCEPTIONS may read as “on” in release builds even though ETL_DEBUG_ASSERTs won’t run.
- Expose a matching etl::traits flag for consistency with using_exceptions.
Suggested tweak within this hunk:
-#if defined(ETL_DEBUG_THROW_EXCEPTIONS) +#if defined(ETL_DEBUG) && defined(ETL_DEBUG_THROW_EXCEPTIONS) #define ETL_DEBUG_USING_EXCEPTIONS 1 #define ETL_DEBUG_NOT_USING_EXCEPTIONS 0 #else #define ETL_DEBUG_USING_EXCEPTIONS 0 #define ETL_DEBUG_NOT_USING_EXCEPTIONS 1 #endifAnd add this introspection constant near the other traits (outside this hunk, shown for clarity):
// In namespace etl::traits, alongside 'using_exceptions' static ETL_CONSTANT bool using_debug_exceptions = (ETL_DEBUG_USING_EXCEPTIONS == 1);Could you confirm error_handler.h branches solely on ETL_IS_DEBUG_BUILD first, then ETL_DEBUG_USING_EXCEPTIONS, so this gating change doesn’t alter behaviour?
test/etl_profile.h (1)
34-36: Make debug-assert throwing opt-in per test configuration (avoid hard-wiring)Defining both ETL_DEBUG and ETL_DEBUG_THROW_EXCEPTIONS here forces “throw-on-debug-assert” for all unit tests. If you plan to test both behaviours (throw vs. assert/terminate), consider moving ETL_DEBUG_THROW_EXCEPTIONS to a dedicated test profile or make it overridable:
-#define ETL_DEBUG -#define ETL_THROW_EXCEPTIONS -#define ETL_DEBUG_THROW_EXCEPTIONS +#define ETL_DEBUG +#define ETL_THROW_EXCEPTIONS +#ifndef ETL_DEBUG_THROW_EXCEPTIONS +#define ETL_DEBUG_THROW_EXCEPTIONS +#endifPlease verify your CI runs at least two configurations:
- Debug + ETL_DEBUG_THROW_EXCEPTIONS (to exercise exception path)
- Debug without ETL_DEBUG_THROW_EXCEPTIONS (to exercise assert path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
include/etl/error_handler.h(1 hunks)include/etl/platform.h(1 hunks)test/etl_profile.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/etl/error_handler.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T14:16:52.778Z
Learnt from: filipe-cuim
PR: ETLCPP/etl#1174
File: include/etl/type_traits.h:1974-1980
Timestamp: 2025-09-01T14:16:52.778Z
Learning: In the ETL library, ETL_USE_TYPE_TRAITS_BUILTINS is a global flag that automatically defines all individual ETL_USING_BUILTIN_IS_* flags (like ETL_USING_BUILTIN_IS_ASSIGNABLE, ETL_USING_BUILTIN_IS_CONSTRUCTIBLE, etc.) in include/etl/profiles/determine_builtin_support.h. Therefore, the per-trait preprocessor guards only need to check their individual flags, not both the individual and global flags.
Applied to files:
test/etl_profile.hinclude/etl/platform.h
📚 Learning: 2025-09-01T14:17:35.337Z
Learnt from: filipe-cuim
PR: ETLCPP/etl#1174
File: include/etl/type_traits.h:1974-1985
Timestamp: 2025-09-01T14:17:35.337Z
Learning: The current implementation in this PR correctly uses only individual trait flags (like ETL_USING_BUILTIN_IS_ASSIGNABLE) in preprocessor guards, not the global ETL_USE_TYPE_TRAITS_BUILTINS flag, because the global flag automatically defines all individual flags when enabled, making dual checks redundant.
Applied to files:
include/etl/platform.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clang C++20 OSX - STL (macos-13)
- GitHub Check: Clang C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - STL
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL - Force C++03 (macos-13)
- GitHub Check: GCC C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL (macos-13)
- GitHub Check: Clang C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
|
Also changed, one of the functions was marked ETL_CONSTEXPR and needed changed to ETL_CONSTEXPR14. This will break backward compatibility. |
| #endif | ||
| #endif | ||
|
|
||
| #if ETL_IS_DEBUG_BUILD |
There was a problem hiding this comment.
I think there should be other macros that make ETL_DEBUG_ASSERT consistent with the options provided by the ETL_ASSERT_XXX set.
There was a problem hiding this comment.
I filled out the rest of the asserts from the ETL_ASSERT_XXX set. I'm not sure if its useful or not but I also included macros for ETL_DEBUG_USE_ASSERT_FUNCTION and ETL_DEBUG_LOG_ERRORS.
OK, I believe I changed this because I was getting a compiler error using the assert macro in C++11 constexpr. I didn't know until just now, that C++11 has some restrictions but it is still possible to throw from constexpr. From cppreference: Maybe in this case we need a slightly different macro expression. I'll experiment later today to see what is possible. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/etl/array.h(12 hunks)include/etl/error_handler.h(1 hunks)include/etl/platform.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/etl/platform.h
- include/etl/error_handler.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: GCC C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++23 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Windows - No STL
- GitHub Check: Windows - STL - Force C++03
🔇 Additional comments (4)
include/etl/array.h (4)
148-149: operator[] (non-const): Debug-assert looks goodBounds check in debug is appropriate and mirrors std semantics while remaining zero-cost in release.
178-179: front/back static_asserts: Nice compile-time guardGood to enforce SIZE > 0 on these accessors in the primary template.
Also applies to: 189-190, 201-202, 212-213
602-604: Docs tweak looks fineThe parameter descriptions read clearly.
161-169: Incorrect suggestion: macro already includes return statement
TheETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPRmacro is defined as#define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) return (b) ? (v) : throw(e)so adding an extra
returnwould produce invalid syntax. No change needed.Likely an incorrect or invalid review comment.
I reverted to |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
include/etl/array.h (1)
449-452: Pre-checks in size_t wrappers avoid UB on begin() + nThese guards resolve the earlier concern about pointer arithmetic occurring before iterator-level checks. Nice defensive improvement.
Also applies to: 480-483, 520-525, 548-553, 575-581, 605-611
🧹 Nitpick comments (1)
include/etl/error_handler.h (1)
414-417: Attribute placement: wrap the return to bind ETL_UNLIKELY to the if-statementWithout braces, ETL_UNLIKELY may attach to the return rather than the if (and can be rejected by some compilers). Mirror the braced style used elsewhere.
- #define ETL_DEBUG_ASSERT_OR_RETURN(b, e) {if (!(b)) ETL_UNLIKELY return;} - #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE(b, e, v) {if (!(b)) ETL_UNLIKELY return(v);} + #define ETL_DEBUG_ASSERT_OR_RETURN(b, e) {if (!(b)) ETL_UNLIKELY { return; }} + #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE(b, e, v) {if (!(b)) ETL_UNLIKELY { return (v); }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/etl/array.h(17 hunks)include/etl/error_handler.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
include/etl/*.h
⚙️ CodeRabbit configuration file
include/etl/*.h: Review the C++ code in these directories. The code must not use
STL containers but must instead rely on Embedded Template Library
(ETL) for data structures and algorithms.When answering questions, provide accurate and concise information based on the ETL documentation and codebase. If you don't know the answer, just say "I don't know". Do not make up answers.
When providing code examples, ensure they are relevant to the ETL and follow its conventions.
Always be polite and professional in your responses.Header files should start with the ETL's standard MIT comment block.
Header files should have include guards in the format '#ifndef ETL_>INCLUDED #define ETL_INCLUDED #endif'
Headers should be organized in the following order
- The first header include should be '#include "platform.h"'
- Headers from the ETL
- Headers from the path 'private/**'
- C++ standard library headers
- C standard library headers
Pay extra attention to the following
- Buffer overflows
- Memory leaks
- Undefined behavior
Check that the following conventions are used
- Variable names should be in
snake_case- Macro constants should be
ALL_CAPS_WITH_UNDERSCORES- Macro constants should be prefixed with 'ETL_'
- Function, class, and type names should be
snake_case- enum members should be
Caps_Snake_CasePlease keep your outputs short and to the point, unless otherwise asked
- Keep your responses short, describe small issues in a few sentences
- Don't output tips, analysis chains or anything else with a collapsible view
- Don't output
comment summariesorfix prompts for AIs- If the highlighted issue is complex, don't suggest a solution
- Don't use emojis
Files:
include/etl/array.hinclude/etl/error_handler.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - STL
- GitHub Check: Windows - STL - Force C++03
- GitHub Check: Clang C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: Windows - No STL - Force C++03
- GitHub Check: Clang C++23 OSX - No STL (macos-13)
- GitHub Check: Clang C++23 OSX - STL - Force C++03 (macos-13)
- GitHub Check: Clang C++23 OSX - STL (macos-13)
- GitHub Check: GCC C++17 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 OSX - STL (macos-13)
- GitHub Check: GCC C++20 Linux - STL (ubuntu-22.04)
🔇 Additional comments (5)
include/etl/array.h (3)
148-150: Good: debug guard on non-const operator[]Adds a clear precondition without impacting release builds.
178-179: Good: compile-time SIZE > 0 checks for front/backPrevents UB at call sites and keeps zero-sized specialisation intact.
Also applies to: 189-190, 201-202, 212-213
461-463: Iterator-range assertions look correctBounds cover cbegin() ≤ first ≤ last ≤ cend() and position < cend(); consistent with the algorithms used.
Also applies to: 494-496, 534-536, 563-565, 589-591, 620-621
include/etl/error_handler.h (2)
367-413: Overall: debug-assert family matches ETL_ASSERT variantsNice symmetry across assert/throw/log/handler permutations; names and prefixes align with ETL conventions.
367-376: Enable assert_handler for debug assertions when ETL_DEBUG_USE_ASSERT_FUNCTION is set
Modify the guard around theassert_handlerdefinitions so it also triggers onETL_DEBUG_USE_ASSERT_FUNCTION:-#elif defined(ETL_USE_ASSERT_FUNCTION) +#elif defined(ETL_USE_ASSERT_FUNCTION) || defined(ETL_DEBUG_USE_ASSERT_FUNCTION)⛔ Skipped due to learnings
Learnt from: filipe-cuim PR: ETLCPP/etl#1174 File: include/etl/type_traits.h:1974-1985 Timestamp: 2025-09-01T14:17:35.361Z Learning: The current implementation in this PR correctly uses only individual trait flags (like ETL_USING_BUILTIN_IS_ASSIGNABLE) in preprocessor guards, not the global ETL_USE_TYPE_TRAITS_BUILTINS flag, because the global flag automatically defines all individual flags when enabled, making dual checks redundant.Learnt from: filipe-cuim PR: ETLCPP/etl#1174 File: include/etl/type_traits.h:1974-1980 Timestamp: 2025-09-01T14:16:52.836Z Learning: In the ETL library, ETL_USE_TYPE_TRAITS_BUILTINS is a global flag that automatically defines all individual ETL_USING_BUILTIN_IS_* flags (like ETL_USING_BUILTIN_IS_ASSIGNABLE, ETL_USING_BUILTIN_IS_CONSTRUCTIBLE, etc.) in include/etl/profiles/determine_builtin_support.h. Therefore, the per-trait preprocessor guards only need to check their individual flags, not both the individual and global flags.
| //throwing from c++11 constexpr requires a special macro | ||
| #if ETL_USING_CPP11 && !ETL_USING_CPP14 && ETL_DEBUG_USING_EXCEPTIONS | ||
| ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(i < SIZE, ETL_ERROR(array_out_of_range), _buffer[i]); | ||
| #else | ||
| ETL_DEBUG_ASSERT(i < SIZE, ETL_ERROR(array_out_of_range)); | ||
|
|
||
| return _buffer[i]; | ||
| #endif | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
C++11 constexpr + debug exceptions + debug logging breaks single-return rule
When ETL_USING_CPP11 && !ETL_USING_CPP14 && ETL_DEBUG_USING_EXCEPTIONS && ETL_DEBUG_LOG_ERRORS, ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR expands to an if + return, which is ill-formed in a C++11 constexpr function body (must be a single return statement). Prefer a single return expression (use comma operator) and move the fix into the macro (see error_handler.h comment with diff).
Enforce single-return in C++11 constexpr
ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR expands to a conditional if (…) return, which violates the C++11 constexpr rule that a function body may only contain exactly one return. Redefine it to a single return expression (e.g. using the comma operator for the assertion) in include/etl/error_handler.h.
🤖 Prompt for AI Agents
In include/etl/array.h around lines 161 to 169, the macro
ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR expands to an if-return form
which violates C++11 constexpr's single-return rule; update the macro definition
in include/etl/error_handler.h so that under ETL_USING_CPP11 && !ETL_USING_CPP14
&& ETL_DEBUG_USING_EXCEPTIONS it expands to a single return statement (for
example, perform the assertion expression as a side-effect using the comma
operator or a conditional operator and then return the value in the same return
expression), ensuring the assertion still occurs and the function body contains
exactly one return.
| #if defined(ETL_DEBUG_LOG_ERRORS) | ||
| #define ETL_DEBUG_ASSERT(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e));}} // If the condition fails, calls the error handler then throws an exception. | ||
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e));} return (b) ? (v) : throw(e) // throwing from c++11 constexpr requires ? operator | ||
| #define ETL_DEBUG_ASSERT_OR_RETURN(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return;}} // If the condition fails, calls the error handler then throws an exception. | ||
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE(b, e, v) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return(v);}} // If the condition fails, calls the error handler then throws an exception. |
There was a problem hiding this comment.
Fix C++11 constexpr variant to remain a single return statement (with logging)
The current macro emits an extra statement in C++11 constexpr contexts when ETL_DEBUG_LOG_ERRORS is set. Keep logging but ensure one return expression.
Apply:
- #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e));} return (b) ? (v) : throw(e) // throwing from c++11 constexpr requires ? operator
+ #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) return (b) ? (v) : (etl::error_handler::error((e)), throw (e), (v)) // single return expr for C++11 constexpr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if defined(ETL_DEBUG_LOG_ERRORS) | |
| #define ETL_DEBUG_ASSERT(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e));}} // If the condition fails, calls the error handler then throws an exception. | |
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e));} return (b) ? (v) : throw(e) // throwing from c++11 constexpr requires ? operator | |
| #define ETL_DEBUG_ASSERT_OR_RETURN(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return;}} // If the condition fails, calls the error handler then throws an exception. | |
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE(b, e, v) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return(v);}} // If the condition fails, calls the error handler then throws an exception. | |
| #if defined(ETL_DEBUG_LOG_ERRORS) | |
| #define ETL_DEBUG_ASSERT(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e));}} // If the condition fails, calls the error handler then throws an exception. | |
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR(b, e, v) return (b) ? (v) : (etl::error_handler::error((e)), throw (e), (v)) // single return expr for C++11 constexpr | |
| #define ETL_DEBUG_ASSERT_OR_RETURN(b, e) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return;}} // If the condition fails, calls the error handler then throws an exception. | |
| #define ETL_DEBUG_ASSERT_OR_RETURN_VALUE(b, e, v) {if (!(b)) ETL_UNLIKELY {etl::error_handler::error((e)); throw((e)); return(v);}} // If the condition fails, calls the error handler then throws an exception. |
🤖 Prompt for AI Agents
In include/etl/error_handler.h around lines 377-381, the
ETL_DEBUG_ASSERT_OR_RETURN_VALUE_CPP11_CONSTEXPR macro currently emits an extra
statement and breaks C++11 constexpr contexts; replace its body with a single
return expression that uses the conditional (?:) operator so that when (b) is
true it returns v, otherwise it first invokes etl::error_handler::error(e) and
then throws e — all inside the single returned expression (ensure proper
parenthesization for the conditional and the error call so the macro remains a
single constexpr-compatible return).
jwellbelove
changed the base branch from
master
to
pull-request/#1175-Debug-assert
jwellbelove
merged commit 999c210
into
ETLCPP:pull-request/#1175-Debug-assert
|
@jwellbelove Thanks for merging. Unless you have other ideas, I would assume that the priority for adding additional checks would be the std equivalent containers first (vector, string, etc). I did start adding some to vector so I can raise another PR for that one when ready. I'll also make note of which classes I am using at my workplace and put them on the todo list as well. |
|
I've been thinking on the use-cases of For example: I was thinking that index operator checks probably fall under the same use case, and should probably have similar code. The idea behind this is to not slow down the debug build unnecessarily. |
I think I follow what you are saying, and that looks like an oversight. It certainly should be allowable to make a debug build without the debug assert being enabled, but I think as currently implemented they are forced on with no way to disable.
I would be also be fine with this. But then what would other checks look like? For example the iterator checks in
I'm not really in favor of removing configuration options. I'm sure some people are already using and happy with it. |
|
I thought about it a bit more, what if we did something like this. at() style assert stays the same, just as reference. I'd like to try to clean this up a bit so what if we include the enable condition in the macro:
Then any checks that don't fit into a category can just be called excessive checks or something like that.
I would think then that if excessive checks is turned on then all the other check conditions would be turned on. The assert when macro would be something like this: Just throwing out an idea! |
|
The only issue I have with this is that, for <C++20, the code does not disappear when |
Maybe then we have a convention of ETL_ASSERT_XXX, for example: Maybe only do this for ETL_ASSERT because it is the simplest and the most common use. Then if there is a need for more complicated logic, the #if logic can be done as it is now. |
|
That looks like a reasonable solution. |
* debug assert POC * Swith to ETL_CONSTEXPR14 * Finish TODO checks * First and last can be equal * Add ETL_DEBUG_THROW_EXCEPTIONS * Try allowing c++11 constexpr * Add macro for throwing from c++11 constexpr * Remove braces * Add extra asserts in size_t overload functions * Fill out debug asserts * Line up comments
I implemented a proof of concept for
ETL_DEBUG_ASSERTdiscussed in #1173 and possibly also a solution to #997 .For the proof of concept I modified expected.h that was already using debug asserts and changed to the new macro. Then I also inserted some new uses of the macro into array.h . I put
//TODOon some functions that looked like they could use debug asserts, but I just haven't worked out the logic yet.Also changed, one of the functions was marked
ETL_CONSTEXPRand needed changed toETL_CONSTEXPR14. Also added static asserts to the front and back function, since I was looking at each function for error checking anyway.For discussion: I think we might want a new macro to enable exceptions for the debug asserts (ex
ETL_DEBUG_USING_EXCEPTIONS). Throwing exceptions is useful for the unit tests, but I think in use most people would want to only callassert(matching libstdc++ behavior).If this looks like its on the right track, then more work can be done to finish the TODOs and convert the rest of the debug asserts to use the new macro.
Edit: Finished array.h debug checks