Introduce Cast to void for Condition of Assert #1191
Conversation
Removed ETL_NOEXCEPT from closureoperator(), call_if(), and call_or()
In addition to removing noexcept from call_if, this is also needed to prevent an abort when cancelling a pthread that is executing a delegate.
Currently, in case we use a parameter of a function _only_ inside of an ETL_ASSERT and the ETL configuration disables the ETL_ASSERT, we get a compiler warning about an unused parameter. Therefore, this change casts the condition of ETL_ASSERT to void.
WalkthroughMacros in include/etl/error_handler.h were modified so that under ETL_NO_CHECKS many assert macros (ETL_ASSERT, ETL_ASSERT_OR_RETURN, ETL_ASSERT_OR_RETURN_VALUE and ETL_ASSERT_FAIL variants) now evaluate to Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)include/etl/*.h⚙️ CodeRabbit configuration file
Files:
⏰ 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)
🔇 Additional comments (2)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/etl/error_handler.h(2 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/error_handler.h
🧠 Learnings (2)
📓 Common 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.
📚 Learning: 2025-09-01T14:17:35.361Z
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.
Applied to files:
include/etl/error_handler.h
include/etl/error_handler.h
Outdated
| #define ETL_ASSERT_FAIL(e) static_cast<void>(b) // Does nothing. | ||
| #define ETL_ASSERT_FAIL_AND_RETURN(e) static_cast<void>(b) // Does nothing. | ||
| #define ETL_ASSERT_FAIL_AND_RETURN_VALUE(e, v) static_cast<void>(b) // Does nothing. | ||
| #elif defined(ETL_USE_ASSERT_FUNCTION) |
There was a problem hiding this comment.
ETL_ASSERT_FAIL macros reference undefined identifier ‘b’
These expand to static_cast(b) but ‘b’ is not a macro parameter here, causing a compile error. They should be true no‑ops and must not evaluate ‘e’.
Apply this diff to align with the existing release branch behaviour (lines 360–363):
- #define ETL_ASSERT_FAIL(e) static_cast<void>(b) // Does nothing.
- #define ETL_ASSERT_FAIL_AND_RETURN(e) static_cast<void>(b) // Does nothing.
- #define ETL_ASSERT_FAIL_AND_RETURN_VALUE(e, v) static_cast<void>(b) // Does nothing.
+ #define ETL_ASSERT_FAIL(e) // Does nothing.
+ #define ETL_ASSERT_FAIL_AND_RETURN(e) {return;} // Returns.
+ #define ETL_ASSERT_FAIL_AND_RETURN_VALUE(e, v) {return (v);} // Returns a value.📝 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.
| #define ETL_ASSERT_FAIL(e) static_cast<void>(b) // Does nothing. | |
| #define ETL_ASSERT_FAIL_AND_RETURN(e) static_cast<void>(b) // Does nothing. | |
| #define ETL_ASSERT_FAIL_AND_RETURN_VALUE(e, v) static_cast<void>(b) // Does nothing. | |
| #elif defined(ETL_USE_ASSERT_FUNCTION) | |
| #define ETL_ASSERT_FAIL(e) // Does nothing. | |
| #define ETL_ASSERT_FAIL_AND_RETURN(e) {return;} // Returns. | |
| #define ETL_ASSERT_FAIL_AND_RETURN_VALUE(e, v) {return (v);} // Returns a value. | |
| #elif defined(ETL_USE_ASSERT_FUNCTION) |
🤖 Prompt for AI Agents
In include/etl/error_handler.h around lines 308 to 311, the ETL_ASSERT_FAIL
macros incorrectly reference an undefined identifier 'b' (static_cast<void>(b))
which causes a compile error; update each macro so it is a true no‑op that does
not evaluate 'e' (match the release branch behavior) by replacing the
static_cast(void)(b) with a safe no‑op such as static_cast<void>(0) (or (void)0)
for ETL_ASSERT_FAIL, ETL_ASSERT_FAIL_AND_RETURN and
ETL_ASSERT_FAIL_AND_RETURN_VALUE.
|
ChatGPT points out that using |
|
Hi @jwellbelove, I am unsure what is the intended behavior, I agree that using
I have no strong preference here, what do you think? |
|
The standard |
|
@jwellbelove , okay, I changed it to |
In case we disable ASSERTs in e.g. non debug builds, we want to expand it to "nothing", similar to how the std assert works. Introducing a cast to void on the conidition would still evaluate it and potentially cause side-effects. Therefore, we use the sizeof operator to ensure the expression is not evaluated in case ASSERTs are disabled.
jwellbelove
changed the base branch from
master
to
pull-request/#1191-Introduce-Cast-to-void-for-Condition-of-Assert
…dition-of-Assert' into chru-do-nothing
jwellbelove
merged commit 018c7ce
into
ETLCPP:pull-request/#1191-Introduce-Cast-to-void-for-Condition-of-Assert
…-of-Assert' into development # Conflicts: # include/etl/error_handler.h
* Removed ETL_NOEXCEPT from delegate operator(), call_if(), and call_or() Removed ETL_NOEXCEPT from closureoperator(), call_if(), and call_or() * Updated version and release notes * Updated version and release notes * Remove noexcept from delegate method stubs. (#1185) In addition to removing noexcept from call_if, this is also needed to prevent an abort when cancelling a pthread that is executing a delegate. * Updated version and release notes * Introduce Cast to void for Condition of Assert Currently, in case we use a parameter of a function _only_ inside of an ETL_ASSERT and the ETL configuration disables the ETL_ASSERT, we get a compiler warning about an unused parameter. Therefore, this change casts the condition of ETL_ASSERT to void. * Use sizeof to avoid evaluation of Expression In case we disable ASSERTs in e.g. non debug builds, we want to expand it to "nothing", similar to how the std assert works. Introducing a cast to void on the conidition would still evaluate it and potentially cause side-effects. Therefore, we use the sizeof operator to ensure the expression is not evaluated in case ASSERTs are disabled. --------- Co-authored-by: John Wellbelove <[email protected]> Co-authored-by: Marco Nilsson <[email protected]> Co-authored-by: John Wellbelove <[email protected]>
4 participants
Currently, in case we use a parameter of a function only inside of an
ETL_ASSERTand the ETL configuration disables the ETL_ASSERT, we get a compiler warning about an unused parameter. Therefore, this change casts the condition of ETL_ASSERT to void.