Guards around usage of std::initializer_list in optional.h #1186
Conversation
In optional.h, ETL_HAS_INITIALIZER_LIST is being used to guard against cases where std::initializer_list is not available. But not consistently. This changes fixes it by adding it in the remaining places.
WalkthroughAdds conditional in-place constructors taking std::initializer_list to include/etl/optional.h for both fundamental and non-fundamental specialisations, implemented in private_optional::optional_impl and exposed via matching public constructors when ETL_HAS_INITIALIZER_LIST is defined. Adjusts preprocessor macro hygiene related to constexpr bool-return macros. Additionally, fixes typographical errors in MIT license headers in include/etl/gcd.h, include/etl/lcm.h, include/etl/private/chrono/duration.h and include/etl/ratio.h. No other API or runtime behaviour changes. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/etl/optional.h (2)
1514-1538: SFINAE gating broken by U being deduced from initializer_list.Here U is deduced from std::initializer_list, so ETL_OPTIONAL_ENABLE_CPP14/CPP20_STL now depend on the list’s element type instead of T. Example: T = etl::vector makes U = int, incorrectly enabling the CPP14 branch.
Use a separate element type parameter (e.g., E) and keep U = T solely for gating.
- template <typename U = T, ETL_OPTIONAL_ENABLE_CPP14, typename... TArgs> + template <typename U = T, ETL_OPTIONAL_ENABLE_CPP14, typename E, typename... TArgs> ETL_CONSTEXPR14 explicit optional(etl::in_place_t, - std::initializer_list<U> ilist, + std::initializer_list<E> ilist, TArgs&&... args) : impl_t(etl::in_place_t{}, ilist, etl::forward<TArgs>(args)...) - template <typename U = T, ETL_OPTIONAL_ENABLE_CPP20_STL, typename... TArgs> + template <typename U = T, ETL_OPTIONAL_ENABLE_CPP20_STL, typename E, typename... TArgs> ETL_CONSTEXPR20_STL explicit optional(etl::in_place_t, - std::initializer_list<U> ilist, + std::initializer_list<E> ilist, TArgs&&... args) : impl_t(etl::in_place_t{}, ilist, etl::forward<TArgs>(args)...)
1125-1135: Wrong member returned in fundamental specialisation.This returns storage.u.value, but storage_type for the fundamental path has no u member. Should return storage.value.
- return storage.u.value; + return storage.value;
🧹 Nitpick comments (1)
include/etl/optional.h (1)
2317-2323: Typo and missing undef lead to macro leak.You undef CPP20 macro twice (second with “COMSTEXPR” typo) and never undef ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14.
-#undef ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL -#undef ETL_OPTIONAL_ENABLE_COMSTEXPR_BOOL_RETURN_CPP20_STL +#undef ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL +#undef ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/etl/optional.h(4 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/optional.h
🧠 Learnings (1)
📓 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.
⏰ 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 - No STL (ubuntu-22.04)
- GitHub Check: Clang C++20 Linux - STL - Force C++03 (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - 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++20 Linux - STL (ubuntu-22.04)
- GitHub Check: GCC C++20 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - 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++11 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++17 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++23 OSX - STL (macos-13)
- GitHub Check: Clang C++14 Linux - No STL (ubuntu-22.04)
- GitHub Check: Clang C++14 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++11 Linux - STL (ubuntu-22.04)
- GitHub Check: Clang C++23 Linux - No STL (ubuntu-22.04)
- GitHub Check: Windows - No STL
- GitHub Check: Windows - STL - Force C++03
🔇 Additional comments (2)
include/etl/optional.h (2)
198-209: Good guard; ensure header is available when enabled.The new initializer_list in-place ctor is correctly wrapped with ETL_HAS_INITIALIZER_LIST. Please verify that <initializer_list> is included whenever this macro is defined; otherwise this will fail to compile on some toolchains.
If it’s not already included elsewhere, add:
#include "placement_new.h" + +#if ETL_HAS_INITIALIZER_LIST +#include <initializer_list> +#endifAlso applies to: 34-42
821-832: Mirrored ctor for fundamental path looks fine.The guarded initializer_list overload mirrors the non‑fundamental path and forwards correctly to storage.construct(...).
rolandreichweinbmw
force-pushed
the
fix-optional-std-initializer_list
branch
from
5e88b54 to
60260a4
Compare
Instead of undefining ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14, ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL was undefined twice (one of the misspelled).
rolandreichweinbmw
force-pushed
the
fix-optional-std-initializer_list
branch
from
60260a4 to
889acf6
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
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/optional.h (1)
1125-1135: Bug: wrong storage member returned in fundamental emplace().
Fundamental storage has ‘value’, not ‘u.value’; this will not compile if used.Apply this diff:
ETL_CONSTEXPR20_STL T& emplace(const optional_impl<T>& other) { #if ETL_IS_DEBUG_BUILD ETL_ASSERT(other.has_value(), ETL_ERROR(optional_invalid)); #endif storage.construct(other.value()); - return storage.u.value; + return storage.value; }
🧹 Nitpick comments (2)
include/etl/gcd.h (2)
31-32: Include guard macro appears misspelt (GDC vs GCD).
Rename to follow the guideline ‘ETL__INCLUDED’.Apply this diff:
-#ifndef ETL_GDC_INCLUDED -#define ETL_GDC_INCLUDED +#ifndef ETL_GCD_INCLUDED +#define ETL_GCD_INCLUDED
34-37: Consider adding platform.h first.
Guidelines require ‘platform.h’ as the first include.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/etl/gcd.h(1 hunks)include/etl/lcm.h(1 hunks)include/etl/optional.h(5 hunks)include/etl/private/chrono/duration.h(1 hunks)include/etl/ratio.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/ratio.hinclude/etl/gcd.hinclude/etl/lcm.hinclude/etl/optional.h
🧠 Learnings (1)
📚 Learning: 2025-09-01T14:16:52.836Z
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.
Applied to files:
include/etl/optional.h
🔇 Additional comments (8)
include/etl/lcm.h (1)
24-24: LGTM: licence header typo fixed.
Change matches the standard MIT wording.include/etl/gcd.h (1)
24-24: LGTM: licence header typo fixed.
Matches the MIT text.include/etl/ratio.h (1)
24-24: LGTM: licence header typo fixed.
No functional changes.include/etl/private/chrono/duration.h (1)
24-24: LGTM: licence header typo fixed.
Text now matches MIT wording.include/etl/optional.h (4)
821-833: Good: fundamental-path in-place ctor also guarded.
Same note: verify <initializer_list> is included under the guard.
1514-1539: Public forwarding ctors for initializer_list look correct and consistently guarded.
Forwarding to impl with identical signature is OK.If CI covers C++11/14 builds with ETL_HAS_INITIALIZER_LIST both on/off, please confirm they pass.
2320-2322: LGTM: macro cleanup for constexpr-bool return helpers.
Undefs are symmetric with the defines above.
198-209: Include the project initializer_list header (guarded)include/etl/initializer_list.h already conditionally includes <initializer_list>; add after the ETL headers in include/etl/optional.h:
#if ETL_HAS_INITIALIZER_LIST
#include "etl/initializer_list.h"
#endif
or confirm optional.h already pulls it in.
* Guards around usage of std::initializer_list in optional.h In optional.h, ETL_HAS_INITIALIZER_LIST is being used to guard against cases where std::initializer_list is not available. But not consistently. This changes fixes it by adding it in the remaining places. * Fixed #undef in optional.h Instead of undefining ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14, ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL was undefined twice (one of the misspelled). * Fix comment typos
* Guards around usage of std::initializer_list in optional.h In optional.h, ETL_HAS_INITIALIZER_LIST is being used to guard against cases where std::initializer_list is not available. But not consistently. This changes fixes it by adding it in the remaining places. * Fixed #undef in optional.h Instead of undefining ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14, ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL was undefined twice (one of the misspelled). * Fix comment typos
* Guards around usage of std::initializer_list in optional.h In optional.h, ETL_HAS_INITIALIZER_LIST is being used to guard against cases where std::initializer_list is not available. But not consistently. This changes fixes it by adding it in the remaining places. * Fixed #undef in optional.h Instead of undefining ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP14, ETL_OPTIONAL_ENABLE_CONSTEXPR_BOOL_RETURN_CPP20_STL was undefined twice (one of the misspelled). * Fix comment typos
2 participants
In optional.h, ETL_HAS_INITIALIZER_LIST is being used to guard against cases where std::initializer_list is not available. But not consistently. This changes fixes it by adding it in the remaining places.