Deduce types traits based on if compiler builtins are present #1174
Conversation
WalkthroughPer-trait builtin gating replaced a single global builtins gate. Individual traits (assignable, constructible, copy/move constructible, trivially-constructible/destructible/copyable/assignable) now select either compiler intrinsics or ETL fallbacks via new ETL_USING_BUILTIN_* macros. Added private_type_traits/void_t fallbacks, a new is_constant_evaluated() API with multiple backends, introduced ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE, removed ETL_CPP11_TYPE_TRAITS_IS_TRIVIAL_SUPPORTED from many profile headers, and made an assertion in deque::repair() unconditional. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/etl/type_traits.h (3)
1989-2014: Same macro-guard issue for is_constructible (C++11 path)This will incorrectly prefer __is_constructible when ETL_USING_BUILTIN_IS_CONSTRUCTIBLE == 0.
-#if defined(ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS) +#if (defined(ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) && ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS)
2032-2071: Same macro-guard issue for is_constructible (pre-C++11 path)Mirroring the C++11 path; this must evaluate the macro’s value, not just its presence.
-#if defined(ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS) +#if (defined(ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) && ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS)
2073-2132: Update is_trivially_constructible guard and intrinsic selection
- Change the guard at include/etl/type_traits.h:2073 to test the macro’s value, not just its presence:
-#if defined(ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS) +#if (ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS)- Inside that block, remove the unconditional GCC branch and instead probe for the preferred builtin, with a fallback:
-#if defined(ETL_COMPILER_GCC)- static ETL_CONSTANT bool value = __has_trivial_constructor(T);
-#else- static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...);
-#endif
+#if defined(__has_builtin)
- #if __has_builtin(__is_trivially_constructible)
- static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...);
- #elif __has_builtin(__has_trivial_constructor)
- static ETL_CONSTANT bool value = __has_trivial_constructor(T);
- #else
- static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...);
- #endif
+#else- static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...);
+#endif</blockquote></details> </blockquote></details>🧹 Nitpick comments (1)
include/etl/type_traits.h (1)
2154-2193: Same macro-guard issue for trivially-copyable traitsWhile unchanged behaviour-wise, do verify that mapping “copy assignable” to “copyable” is acceptable for your no-STL profile. If precision matters, consider a dedicated probe (e.g., __is_trivially_assignable where supported).
-#if defined(ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS) +#if (defined(ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE) && ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE) || defined(ETL_USE_TYPE_TRAITS_BUILTINS)📜 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 (1)
include/etl/type_traits.h(7 hunks)🧰 Additional context used
🧬 Code graph analysis (1)
include/etl/type_traits.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(111-124)🔇 Additional comments (3)
include/etl/type_traits.h (3)
1833-1966: User-defined traits branch looks sensible; confirm surface area matches expectationsThis section cleanly forces user specialisations for non-arithmetic/pointer types. Please confirm that downstream code never relies on partial meta-functions like is_trivially_copy_constructible in this branch for pre-C++11, where only the arithmetic/pointer defaults are available.
2113-2116: is_trivially_copy_constructible piggybacks on is_trivially_constructible; ensure the latter respects T const&This composition is fine provided the underlying is_trivially_constructible honours the argument list (see previous comment). Otherwise, results will be wrong on GCC when __has_trivial_constructor(T) is used.
1974-2193: ETL_USING_BUILTIN_ macro usage verified*
AllETL_USING_BUILTIN_*checks ininclude/etl/type_traits.hcorrespond to 0/1 definitions ininclude/etl/profiles/determine_builtin_support.h. No regressions detected.
filipe-cuim
force-pushed
the
deduce_if_builtins_are_present
branch
from
59715bc to
bdeca28
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/etl/type_traits.h (1)
2032-2071: Non-variadic (pre-C++11) is_constructible should also honor the global flagSame gating issue in the non-variadic branch.
-#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE +#if (ETL_USE_TYPE_TRAITS_BUILTINS) || (ETL_USING_BUILTIN_IS_CONSTRUCTIBLE) // is_constructible template<typename T, typename TArgs = void> struct is_constructible { static ETL_CONSTANT bool value = __is_constructible(T, TArgs); }; // is_constructible<T, void> template<typename T> struct is_constructible<T, void> { static ETL_CONSTANT bool value = __is_constructible(T); }; // is_copy_constructible / is_move_constructible unchanged #else
♻️ Duplicate comments (1)
include/etl/type_traits.h (1)
2133-2152: Prefer __is_trivially_destructible over GCC __has_trivial_destructorThis was raised previously and is still present; unify on the modern builtin when the trait is enabled.
-#if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE +#if (ETL_USE_TYPE_TRAITS_BUILTINS) || (ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE) // is_trivially_destructible template <typename T> struct is_trivially_destructible { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_destructor(T); -#else static ETL_CONSTANT bool value = __is_trivially_destructible(T); -#endif }; #else
🧹 Nitpick comments (2)
include/etl/type_traits.h (2)
2167-2177: Minor: unify GCC path for is_trivially_copyableGCC supports __is_trivially_copyable; the __has_trivial_copy fallback is unnecessary when the trait is declared available.
// is_trivially_copyable template <typename T> struct is_trivially_copyable { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_copy(T); -#else static ETL_CONSTANT bool value = __is_trivially_copyable(T); -#endif };
1988-2013: Include global builtins flag in is_constructible guard
Change in include/etl/type_traits.h:-#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE +#if (ETL_USE_TYPE_TRAITS_BUILTINS) || (ETL_USING_BUILTIN_IS_CONSTRUCTIBLE)Ensures ETL_USE_TYPE_TRAITS_BUILTINS forces use of compiler built-ins for is_constructible, matching other traits.
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
include/etl/type_traits.h(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/etl/type_traits.h (2)
include/etl/platform.h (1)
etl(559-625)include/etl/profiles/determine_builtin_support.h (1)
etl(111-124)
🔇 Additional comments (1)
include/etl/type_traits.h (1)
1833-1833: LGTM on user-defined traits branch selectionSwitching on ETL_USER_DEFINED_TYPE_TRAITS here is correct and keeps the fallback isolated.
include/etl/type_traits.h
Outdated
| #if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE | ||
| //********************************************* | ||
| // is_trivially_copy_assignable | ||
| template <typename T> | ||
| struct is_trivially_copy_assignable | ||
| { | ||
| #if defined(ETL_COMPILER_GCC) | ||
| static ETL_CONSTANT bool value = __has_trivial_copy(T); | ||
| #else | ||
| static ETL_CONSTANT bool value = __is_trivially_copyable(T); | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
is_trivially_copy_assignable uses the wrong builtin
Using __is_trivially_copyable conflates assignability with copyability. Use __is_trivially_assignable(T&, const T&).
-#if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE
+#if (ETL_USE_TYPE_TRAITS_BUILTINS) || (ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE)
// is_trivially_copy_assignable
template <typename T>
struct is_trivially_copy_assignable
{
-#if defined(ETL_COMPILER_GCC)
- static ETL_CONSTANT bool value = __has_trivial_copy(T);
-#else
- static ETL_CONSTANT bool value = __is_trivially_copyable(T);
-#endif
+ static ETL_CONSTANT bool value =
+#if defined(ETL_COMPILER_CLANG) || defined(ETL_COMPILER_GCC)
+ __is_trivially_assignable(typename add_lvalue_reference<T>::type,
+ typename add_lvalue_reference<const T>::type);
+#else
+ /* Fallback: best-effort approximation */
+ __is_trivially_assignable(typename add_lvalue_reference<T>::type,
+ typename add_lvalue_reference<const T>::type);
+#endif
};If desired, I can add a new feature flag ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE in determine_builtin_support.h and wire this gate to it for stricter portability.
🏁 Script executed:
#!/usr/bin/env bash
set -e
echo "Searching for ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE in repository:"
rg -n "ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE" -C3 .
echo
echo "Searching for ETL_USE_TYPE_TRAITS_BUILTINS in repository:"
rg -n "ETL_USE_TYPE_TRAITS_BUILTINS" -C3 .
echo
echo "Inspecting determine_builtin_support.h for TRIVIALLY_COPYABLE support:"
rg -n "TRIVIALLY_COPYABLE" -C3 include/etl/determine_builtin_support.hLength of output: 68608
Use the proper “trivially assignable” builtin and gate it correctly
- In include/etl/type_traits.h (lines 2154–2165), change
to
#if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLEand replace the body with:#if defined(ETL_USE_TYPE_TRAITS_BUILTINS) || ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLEstatic ETL_CONSTANT bool value = #if defined(ETL_COMPILER_GCC) || defined(ETL_COMPILER_CLANG) __is_trivially_assignable( typename add_lvalue_reference<T>::type, typename add_lvalue_reference<const T>::type); #else /* Fallback: best-effort */ __has_trivial_copy(T) || __is_trivially_copyable(T); #endif
- In include/etl/profiles/determine_builtin_support.h, add before line 36:
#if !defined(ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE) #define ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE \ (__has_builtin(__is_trivially_assignable)) #endif
- In test/test_type_traits.cpp, under TEST(test_is_trivially_copy_assignable), remove the skip guard
so that the test covers the new builtin on all platforms.
#if defined(ETL_COMPILER_GCC) && defined(ETL_USE_TYPE_TRAITS_BUILTINS)
🤖 Prompt for AI Agents
In include/etl/type_traits.h around lines 2154–2165, the compile-time guard and
implementation for is_trivially_copy_assignable must be updated to use the
proper "trivially assignable" builtin and allow it when
ETL_USE_TYPE_TRAITS_BUILTINS is defined; change the top-level if to check
"defined(ETL_USE_TYPE_TRAITS_BUILTINS) ||
ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE" and replace the body so that for GCC or
Clang it uses __is_trivially_assignable with add_lvalue_reference<T>::type and
add_lvalue_reference<const T>::type, and otherwise falls back to a best-effort
check using __has_trivial_copy(T) || __is_trivially_copyable(T). Also, in
include/etl/profiles/determine_builtin_support.h (before line 36) add a guard to
define ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE when
__has_builtin(__is_trivially_assignable) is available, and in
test/test_type_traits.cpp remove the conditional skip guard around
TEST(test_is_trivially_copy_assignable) so the test runs on all platforms to
cover the new builtin.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
include/etl/type_traits.h (2)
2158-2182: Bug: is_trivially_copy_assignable uses “copyable” builtin (semantic mismatch)Trivially copyable ≠ trivially copy-assignable. Use
__is_trivially_assignable(T&, const T&)when available.-#if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE - // is_trivially_copy_assignable - template <typename T> - struct is_trivially_copy_assignable - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_copy(T); -#else - static ETL_CONSTANT bool value = __is_trivially_copyable(T); -#endif - }; +#if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE + // is_trivially_copy_assignable + template <typename T> + struct is_trivially_copy_assignable + { +#if defined(ETL_COMPILER_CLANG) || defined(ETL_COMPILER_GCC) || defined(ETL_COMPILER_MICROSOFT) + static ETL_CONSTANT bool value = + __is_trivially_assignable(typename add_lvalue_reference<T>::type, + typename add_lvalue_reference<const T>::type); +#else + // Best-effort approximation when assignable builtin is unavailable + static ETL_CONSTANT bool value = __is_trivially_copyable(T); +#endif + };If preferred, I can wire this to a dedicated flag
ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLEin determine_builtin_support.h to keep detection centralized.
2077-2121: Prefer __is_trivially_constructible over legacy __has_trivial_constructor
__has_trivial_constructoris legacy and diverges from standard semantics. Use__is_trivially_constructibleon GCC/Clang/MSVC.-#if ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE -#if ETL_USING_CPP11 - template <typename T, typename... TArgs> - struct is_trivially_constructible - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else - static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...); -#endif - }; -#else - template <typename T, typename TArgs = void> - struct is_trivially_constructible - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else - static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs); -#endif - }; - - template <typename T> - struct is_trivially_constructible<T, void> - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else - static ETL_CONSTANT bool value = __is_trivially_constructible(T); -#endif - }; -#endif +#if ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE +#if ETL_USING_CPP11 + template <typename T, typename... TArgs> + struct is_trivially_constructible : etl::bool_constant<__is_trivially_constructible(T, TArgs...)> + { + }; +#else + template <typename T, typename TArgs = void> + struct is_trivially_constructible : etl::bool_constant<__is_trivially_constructible(T, TArgs)> + { + }; + + template <typename T> + struct is_trivially_constructible<T, void> : etl::bool_constant<__is_trivially_constructible(T)> + { + }; +#endif
🧹 Nitpick comments (4)
include/etl/type_traits.h (4)
1974-1985: Make is_assignable a proper trait (derive from bool_constant)Current struct only exposes
value; deriving frometl::bool_constant<...>aligns with the rest of ETL traits and preserves integral_constant semantics.-#if ETL_USING_BUILTIN_IS_ASSIGNABLE - template<typename T1, typename T2> - struct is_assignable - { - static ETL_CONSTANT bool value = __is_assignable(T1, T2); - }; +#if ETL_USING_BUILTIN_IS_ASSIGNABLE + template<typename T1, typename T2> + struct is_assignable : etl::bool_constant<__is_assignable(T1, T2)> + { + };
1988-2013: Unify builtin is_constructible to integral_constant styleSame consistency point: derive from
etl::bool_constantso downstream meta-programs that expect integral_constant semantics keep working.-#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE - // is_constructible - template<typename T, typename... TArgs> - struct is_constructible - { - static ETL_CONSTANT bool value = __is_constructible(T, TArgs...); - }; +#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE + // is_constructible + template<typename T, typename... TArgs> + struct is_constructible : etl::bool_constant<__is_constructible(T, TArgs...)> + { + };
2032-2060: Pre-C++11 builtin is_constructible: also derive from bool_constantKeeps trait shape consistent across language modes.
-#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE - // is_constructible - template<typename T, typename TArgs = void> - struct is_constructible - { - static ETL_CONSTANT bool value = __is_constructible(T, TArgs); - }; - - // is_constructible - template<typename T> - struct is_constructible<T, void> - { - static ETL_CONSTANT bool value = __is_constructible(T); - }; +#if ETL_USING_BUILTIN_IS_CONSTRUCTIBLE + // is_constructible + template<typename T, typename TArgs = void> + struct is_constructible : etl::bool_constant<__is_constructible(T, TArgs)> + { + }; + + // is_constructible<T, void> + template<typename T> + struct is_constructible<T, void> : etl::bool_constant<__is_constructible(T)> + { + };
2171-2181: Minor: make is_trivially_copyable derive from bool_constantCosmetic but consistent with other traits after refactors above.
- template <typename T> - struct is_trivially_copyable - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_copy(T); -#else - static ETL_CONSTANT bool value = __is_trivially_copyable(T); -#endif - }; + template <typename T> + struct is_trivially_copyable : etl::bool_constant< +#if defined(ETL_COMPILER_GCC) + __has_trivial_copy(T) +#else + __is_trivially_copyable(T) +#endif + > + { + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
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 (1)
include/etl/type_traits.h(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
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.
📚 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/type_traits.h
📚 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:
include/etl/type_traits.h
🧬 Code graph analysis (1)
include/etl/type_traits.h (2)
include/etl/platform.h (1)
etl(559-625)include/etl/profiles/determine_builtin_support.h (1)
etl(111-124)
🔇 Additional comments (1)
include/etl/type_traits.h (1)
1833-1966: User-defined traits branch: approach looks correctThe refactor keeps the “user supplies specializations” path intact and conservative (true for arithmetic/pointers only). No issues spotted.
| #if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE | ||
| //********************************************* | ||
| // is_trivially_destructible | ||
| template <typename T> | ||
| struct is_trivially_destructible | ||
| { | ||
| #if defined(ETL_COMPILER_GCC) | ||
| static ETL_CONSTANT bool value = __has_trivial_destructor(T); | ||
| #else | ||
| static ETL_CONSTANT bool value = __is_trivially_destructible(T); | ||
| #endif | ||
| }; | ||
| #else | ||
| //********************************************* | ||
| // is_trivially_destructible | ||
| template <typename T> | ||
| struct is_trivially_destructible : public etl::bool_constant<etl::is_arithmetic<T>::value || etl::is_pointer<T>::value> | ||
| { | ||
| }; | ||
| #endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer __is_trivially_destructible over legacy __has_trivial_destructor
Same rationale as constructible; switch to the modern intrinsic and derive from bool_constant.
-#if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE
- template <typename T>
- struct is_trivially_destructible
- {
-#if defined(ETL_COMPILER_GCC)
- static ETL_CONSTANT bool value = __has_trivial_destructor(T);
-#else
- static ETL_CONSTANT bool value = __is_trivially_destructible(T);
-#endif
- };
+#if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE
+ template <typename T>
+ struct is_trivially_destructible : etl::bool_constant<__is_trivially_destructible(T)>
+ {
+ };📝 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 ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE | |
| //********************************************* | |
| // is_trivially_destructible | |
| template <typename T> | |
| struct is_trivially_destructible | |
| { | |
| #if defined(ETL_COMPILER_GCC) | |
| static ETL_CONSTANT bool value = __has_trivial_destructor(T); | |
| #else | |
| static ETL_CONSTANT bool value = __is_trivially_destructible(T); | |
| #endif | |
| }; | |
| #else | |
| //********************************************* | |
| // is_trivially_destructible | |
| template <typename T> | |
| struct is_trivially_destructible : public etl::bool_constant<etl::is_arithmetic<T>::value || etl::is_pointer<T>::value> | |
| { | |
| }; | |
| #endif | |
| #if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE | |
| //********************************************* | |
| // is_trivially_destructible | |
| template <typename T> | |
| struct is_trivially_destructible : etl::bool_constant<__is_trivially_destructible(T)> | |
| { | |
| }; | |
| #else | |
| //********************************************* | |
| // is_trivially_destructible | |
| template <typename T> | |
| struct is_trivially_destructible | |
| : public etl::bool_constant<etl::is_arithmetic<T>::value || etl::is_pointer<T>::value> | |
| { | |
| }; | |
| #endif |
🤖 Prompt for AI Agents
In include/etl/type_traits.h around lines 2137 to 2156, the GCC branch uses the
legacy __has_trivial_destructor and a static value member; replace that with the
modern __is_trivially_destructible intrinsic and implement the trait by deriving
from etl::bool_constant. Concretely, for the ETL_COMPILER_GCC case change the
static ETL_CONSTANT bool value = __has_trivial_destructor(T); to inherit from
etl::bool_constant<__is_trivially_destructible(T)> (or
etl::bool_constant<__is_trivially_destructible(T) != 0> if needed for type), so
the trait matches the non-GCC branch style and uses the modern intrinsic.
|
Hey @jwellbelove! Do you think it could make sense to add this? |
|
You will need to update |
|
thanks for answer @jwellbelove, will do that then. |
filipe-cuim
force-pushed
the
deduce_if_builtins_are_present
branch
from
fe17c80 to
8ec0776
Compare
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/generators/type_traits_generator.h (2)
2316-2322: common_type<T1,T2> decay path uses T2 twiceShould decay both T1 and T2 in the fallback.
- common_type<typename etl::decay<T2>::type, - typename etl::decay<T2>::type>>::type + common_type<typename etl::decay<T1>::type, + typename etl::decay<T2>::type>>::type
2239-2263: <!-- [scratchpad_update]
[observations updated]
- std::is_default_constructible takes single parameter T (sources 8-1, 9-1, 9-18)
- std::is_trivially_constructible takes T plus optional argument types: "std::is_trivially_constructible_v<T, Args>" (source 2-2, 2-12, 3-1)
- Example from cppreference (source 2-2, 2-12): std::is_trivially_constructible_v<Foo, const Foo&>, std::is_trivially_constructible_v<Foo, int>, std::is_trivially_constructible_v<Foo, int, double> - ALL with multiple parameters
- is_trivially_constructible supports variable arg parameters per standard
[analysis updated]
The review comment is correct:
- is_default_constructible_v incorrectly has TArgs... - should only have T (matches std lib)
- is_trivially_constructible_v should have T, typename... TArgs (matches std lib which supports arg types)
[scratchpad_end] -->Based on the web search results, std::is_default_constructible accepts a single parameter T, whereas std::is_trivially_constructible supports variable argument types (e.g., std::is_trivially_constructible_v<T, Args>).
The review comment accurately identifies the arity mismatches in the template variable declarations. The fixes specified are correct and necessary to align with the standard library behaviour.
Apply the suggested fixes to is_default_constructible_v and is_trivially_constructible_v variable templates.
Remove
TArgs...fromis_default_constructible_vtemplate and addtypename... TArgstois_trivially_constructible_vtemplate.- template<typename T, typename... TArgs> - inline constexpr bool is_default_constructible_v = etl::is_default_constructible<T, TArgs...>::value; + template<typename T> + inline constexpr bool is_default_constructible_v = etl::is_default_constructible<T>::value; … - template <typename T> - inline constexpr bool is_trivially_constructible_v = etl::is_trivially_constructible<T>::value; + template <typename T, typename... TArgs> + inline constexpr bool is_trivially_constructible_v = etl::is_trivially_constructible<T, TArgs...>::value;
🧹 Nitpick comments (1)
include/etl/generators/type_traits_generator.h (1)
1995-2021: is_constructible fallback misses braced-init detectionThe void_t/paren-construct check omits T{Args...}. Consider extending to also test braced initialisation to better mirror std::is_constructible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/etl/generators/type_traits_generator.h(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/type_traits_generator.h
🧠 Learnings (3)
📓 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.
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.
📚 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/generators/type_traits_generator.h
📚 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/generators/type_traits_generator.h
🧬 Code graph analysis (1)
include/etl/generators/type_traits_generator.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(151-169)
🔇 Additional comments (7)
include/etl/generators/type_traits_generator.h (7)
745-769: is_enum SFINAE looks goodRobust detection using static_cast SFINAE plus class/arithmetic/reference exclusions. LGTM.
1576-1597: No actionComment-only tweak around nth_base; nothing to review here.
1840-1973: User-defined traits branch retained sensiblyClear “user must specialise” path with conservative arithmetic/pointer defaults. Consistent with the library’s approach.
1976-1992: Per-trait builtin gating is correctUsing only ETL_USING_BUILTIN_IS_* (no global OR) is the right shape. Keep it this way. Based on learnings.
2145-2156: Prefer __is_trivially_destructible over __has_trivial_destructorAlign GCC with the intrinsic that matches the trait’s semantics.
template <typename T> struct is_trivially_destructible { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_destructor(T); -#else static ETL_CONSTANT bool value = __is_trivially_destructible(T); -#endif };⛔ 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.
2084-2121: ---Files matching
include/etl/generators/*.hare excluded from review per project guidelines. This review comment targetsinclude/etl/generators/type_traits_generator.h, which falls under this exclusion.The underlying concern regarding
__has_trivial_constructoris valid (confirmed: it appears at lines 2092, 2104, 2116 in the file), but this file should not be reviewed. However, the identical issue exists ininclude/etl/type_traits.h(lines 2085, 2097, 2109), which is not excluded and may warrant separate attention.Likely an incorrect or invalid review comment.
2166-2189: Use __is_trivially_assignable for is_trivially_copy_assignableThe trait __has_trivial_assign has been deprecated in favour of __is_trivially_assignable, and the current implementation incorrectly uses __has_trivial_copy (which tests copyability, not assignability). Update to use __is_trivially_assignable(T&, const T&):
#if ETL_USING_BUILTIN_IS_TRIVIALLY_COPYABLE //********************************************* // is_trivially_copy_assignable template <typename T> struct is_trivially_copy_assignable { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_copy(T); -#else - static ETL_CONSTANT bool value = __is_trivially_copyable(T); -#endif + static ETL_CONSTANT bool value = __is_trivially_assignable(T&, const T&); };Consider adding ETL_USING_BUILTIN_IS_TRIVIALLY_ASSIGNABLE in determine_builtin_support.h if some older compilers lack this builtin, and gate this section accordingly.
⛔ 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
include/etl/type_traits.h (3)
2077-2120: Consider preferring__is_trivially_constructibleuniformlyThe GCC branches use the legacy
__has_trivial_constructorintrinsic. Modern GCC supports__is_trivially_constructible, which would provide more consistent behaviour across compilers and better align with the trait semantics (considering constructor arguments).
2137-2156: Consider using__is_trivially_destructibleuniformlyThe GCC branch uses
__has_trivial_destructor, whilst the modern intrinsic__is_trivially_destructibleis available and provides consistent semantics across compilers.
2158-2169:is_trivially_copy_assignableuses copyable intrinsic instead of assignableLine 2167 uses
__is_trivially_copyableto determine copy assignability, which checks whether a type is trivially copyable rather than whether copy assignment is trivial. Consider using__is_trivially_assignable(T&, const T&)when available for more accurate semantics.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/etl/generators/type_traits_generator.h(12 hunks)include/etl/type_traits.h(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/type_traits_generator.h
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/type_traits.h
🧠 Learnings (3)
📓 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.
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.
📚 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/generators/type_traits_generator.hinclude/etl/type_traits.h
📚 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/generators/type_traits_generator.hinclude/etl/type_traits.h
🧬 Code graph analysis (2)
include/etl/generators/type_traits_generator.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(183-202)
include/etl/type_traits.h (2)
include/etl/profiles/determine_builtin_support.h (1)
etl(183-202)include/etl/platform.h (1)
etl(593-659)
🔇 Additional comments (3)
include/etl/type_traits.h (3)
1974-1985: LGTM - Correct per-trait builtin selectionThe implementation correctly checks the individual builtin flag and provides an appropriate conservative fallback for arithmetic and pointer types.
Based on learnings
1988-2013: LGTM - Correct C++11 constructibility detectionThe builtin path correctly delegates to
__is_constructible, whilst the fallback uses proper SFINAE-based detection withvoid_tanddecltype.
2032-2075: LGTM - Non-C++11 constructibility handlingThe non-C++11 path correctly handles both default construction (void args) and single-argument construction through template specialisation. Conservative fallback appropriately limits support to arithmetic and pointer types.
filipe-cuim
force-pushed
the
deduce_if_builtins_are_present
branch
2 times, most recently
from
c7e8d80 to
59c9cad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
include/etl/type_traits.h (3)
2073-2116: Consider using __is_trivially_constructible uniformly.Lines 2080-2084, 2092-2096, and 2104-2108 use GCC-specific
__has_trivial_constructor, whilst non-GCC paths use__is_trivially_constructible. Modern GCC versions support the latter, making the distinction potentially unnecessary.
2133-2144: Consider using __is_trivially_destructible uniformly.Line 2140 uses GCC-specific
__has_trivial_destructor, whilst line 2142 uses__is_trivially_destructible. Modern GCC supports the latter, so the GCC-specific path may be legacy code.
2154-2165: Consider using __is_trivially_assignable uniformly.Line 2161 uses GCC-specific
__has_trivial_assign, whilst line 2163 uses__is_trivially_assignable. Modern GCC supports the latter, making the distinction potentially unnecessary.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
include/etl/generators/type_traits_generator.h(14 hunks)include/etl/profiles/determine_builtin_support.h(1 hunks)include/etl/type_traits.h(13 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
include/etl/profiles/*.h
⚙️ CodeRabbit configuration file
include/etl/profiles/*.h: Ingnore files in this directory
Files:
include/etl/profiles/determine_builtin_support.h
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/type_traits_generator.h
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/type_traits.h
🧠 Learnings (3)
📓 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.
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.
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
🧬 Code graph analysis (2)
include/etl/generators/type_traits_generator.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(187-206)
include/etl/type_traits.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(187-206)
🔇 Additional comments (5)
include/etl/generators/type_traits_generator.h (2)
733-787: LGTM: is_enum implementation formatting cleanup.The formatting adjustments to
is_convertible_to_intandis_enummaintain the existing logic whilst improving consistency.
2480-2507: LGTM: is_constant_evaluated() utility added correctly.The function provides appropriate fallback paths:
- C++23
constevalwhen available- Compiler builtin for C++20
falsedefault for older standardsinclude/etl/type_traits.h (3)
733-787: LGTM: is_enum implementation formatting cleanup.The formatting changes to
is_convertible_to_intand reference exclusion inis_enummaintain existing logic whilst improving readability.
1974-1985: LGTM: Per-trait gating for is_assignable correctly implemented.The builtin check uses
ETL_USING_BUILTIN_IS_ASSIGNABLEas intended, with an appropriate fallback for arithmetic and pointer types.Based on learnings
2480-2500: LGTM: is_constant_evaluated() utility added with appropriate fallback paths.The implementation correctly provides C++23
consteval, builtin, and fallback paths.
filipe-cuim
force-pushed
the
deduce_if_builtins_are_present
branch
from
59c9cad to
b021c4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
include/etl/type_traits.h (2)
2051-2060: Prefer modern intrinsic for trivially constructible; preserve TArgs.__has_trivial_constructor ignores constructor arguments and is deprecated. Use __is_trivially_constructible consistently.
#if ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE #if ETL_USING_CPP11 //********************************************* // is_trivially_constructible template <typename T, typename... TArgs> struct is_trivially_constructible { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...); -#endif }; #else //********************************************* // is_trivially_constructible template <typename T, typename TArgs = void> struct is_trivially_constructible { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs); -#endif }; //********************************************* // is_trivially_constructible template <typename T> struct is_trivially_constructible<T, void> { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_constructor(T); -#else static ETL_CONSTANT bool value = __is_trivially_constructible(T); -#endif }; #endifAlso applies to: 2064-2085
2112-2119: Use __is_trivially_destructible; drop legacy GCC path.Modern compilers support __is_trivially_destructible; __has_trivial_destructor is legacy.
#if ETL_USING_BUILTIN_IS_TRIVIALLY_DESTRUCTIBLE //********************************************* // is_trivially_destructible template <typename T> - struct is_trivially_destructible - { -#if defined(ETL_COMPILER_GCC) - static ETL_CONSTANT bool value = __has_trivial_destructor(T); -#else - static ETL_CONSTANT bool value = __is_trivially_destructible(T); -#endif - }; + struct is_trivially_destructible + { + static ETL_CONSTANT bool value = __is_trivially_destructible(T); + }; #else
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
include/etl/deque.h(2 hunks)include/etl/generators/type_traits_generator.h(15 hunks)include/etl/profiles/arduino_arm.h(0 hunks)include/etl/profiles/armv5.h(0 hunks)include/etl/profiles/armv5_no_stl.h(0 hunks)include/etl/profiles/armv6.h(0 hunks)include/etl/profiles/armv6_no_stl.h(0 hunks)include/etl/profiles/armv7.h(0 hunks)include/etl/profiles/armv7_no_stl.h(0 hunks)include/etl/profiles/cpp03.h(0 hunks)include/etl/profiles/cpp03_no_stl.h(0 hunks)include/etl/profiles/cpp11.h(0 hunks)include/etl/profiles/cpp11_no_stl.h(0 hunks)include/etl/profiles/cpp14.h(0 hunks)include/etl/profiles/cpp14_no_stl.h(0 hunks)include/etl/profiles/cpp17.h(0 hunks)include/etl/profiles/cpp17_no_stl.h(0 hunks)include/etl/profiles/determine_builtin_support.h(4 hunks)include/etl/profiles/determine_compiler_language_support.h(0 hunks)include/etl/profiles/segger_gcc_stlport.h(0 hunks)include/etl/profiles/ticc.h(0 hunks)include/etl/profiles/ticc_no_stl.h(0 hunks)include/etl/type_traits.h(14 hunks)
💤 Files with no reviewable changes (19)
- include/etl/profiles/cpp14_no_stl.h
- include/etl/profiles/cpp17_no_stl.h
- include/etl/profiles/armv5.h
- include/etl/profiles/cpp03_no_stl.h
- include/etl/profiles/arduino_arm.h
- include/etl/profiles/cpp17.h
- include/etl/profiles/cpp03.h
- include/etl/profiles/armv6_no_stl.h
- include/etl/profiles/cpp14.h
- include/etl/profiles/cpp11.h
- include/etl/profiles/armv6.h
- include/etl/profiles/segger_gcc_stlport.h
- include/etl/profiles/armv5_no_stl.h
- include/etl/profiles/determine_compiler_language_support.h
- include/etl/profiles/armv7_no_stl.h
- include/etl/profiles/cpp11_no_stl.h
- include/etl/profiles/ticc_no_stl.h
- include/etl/profiles/armv7.h
- include/etl/profiles/ticc.h
🧰 Additional context used
📓 Path-based instructions (3)
include/etl/profiles/*.h
⚙️ CodeRabbit configuration file
include/etl/profiles/*.h: Ingnore files in this directory
Files:
include/etl/profiles/determine_builtin_support.h
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/deque.hinclude/etl/type_traits.h
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/type_traits_generator.h
🧠 Learnings (3)
📓 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.
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.
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
🧬 Code graph analysis (2)
include/etl/generators/type_traits_generator.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(195-215)
include/etl/type_traits.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(195-215)
🔇 Additional comments (1)
include/etl/deque.h (1)
2487-2490: Confirm unconditional trivially_copyable assert in repair().This now always requires etl::is_trivially_copyable::value. On targets without builtins the fallback only returns true for arithmetic/pointers, so trivially copyable PODs may fail this assert. Is this intentional for all no‑STL profiles?
Delete ETL_CPP11_TYPE_TRAITS_IS_TRIVIAL_SUPPORTED since it was always being evaluated to if Cpp11 was supported.
filipe-cuim
force-pushed
the
deduce_if_builtins_are_present
branch
from
b021c4a to
171ad02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
include/etl/type_traits.h (1)
2048-2060: Prefer modern intrinsics; drop legacy _has_trivial where possible.*To reduce divergence across compilers and simplify maintenance, consider standardising on:
- __is_trivially_constructible for constructibility paths,
- __is_trivially_destructible for destructibility, and
- __is_trivially_copyable for copyability,
instead of GCC-specific __has_trivial_constructor/__has_trivial_destructor/__has_trivial_copy branches. Behaviour should remain equivalent on supported GCC/Clang.
Also applies to: 2062-2085, 2108-2120, 2151-2163
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
include/etl/deque.h(2 hunks)include/etl/generators/type_traits_generator.h(15 hunks)include/etl/profiles/arduino_arm.h(0 hunks)include/etl/profiles/armv5.h(0 hunks)include/etl/profiles/armv5_no_stl.h(0 hunks)include/etl/profiles/armv6.h(0 hunks)include/etl/profiles/armv6_no_stl.h(0 hunks)include/etl/profiles/armv7.h(0 hunks)include/etl/profiles/armv7_no_stl.h(0 hunks)include/etl/profiles/cpp03.h(0 hunks)include/etl/profiles/cpp03_no_stl.h(0 hunks)include/etl/profiles/cpp11.h(0 hunks)include/etl/profiles/cpp11_no_stl.h(0 hunks)include/etl/profiles/cpp14.h(0 hunks)include/etl/profiles/cpp14_no_stl.h(0 hunks)include/etl/profiles/cpp17.h(0 hunks)include/etl/profiles/cpp17_no_stl.h(0 hunks)include/etl/profiles/determine_builtin_support.h(4 hunks)include/etl/profiles/determine_compiler_language_support.h(0 hunks)include/etl/profiles/segger_gcc_stlport.h(0 hunks)include/etl/profiles/ticc.h(0 hunks)include/etl/profiles/ticc_no_stl.h(0 hunks)include/etl/type_traits.h(14 hunks)
💤 Files with no reviewable changes (19)
- include/etl/profiles/cpp03.h
- include/etl/profiles/armv5.h
- include/etl/profiles/cpp17_no_stl.h
- include/etl/profiles/cpp14.h
- include/etl/profiles/armv7_no_stl.h
- include/etl/profiles/ticc.h
- include/etl/profiles/armv6_no_stl.h
- include/etl/profiles/cpp14_no_stl.h
- include/etl/profiles/arduino_arm.h
- include/etl/profiles/cpp17.h
- include/etl/profiles/cpp11_no_stl.h
- include/etl/profiles/cpp03_no_stl.h
- include/etl/profiles/armv6.h
- include/etl/profiles/cpp11.h
- include/etl/profiles/determine_compiler_language_support.h
- include/etl/profiles/armv5_no_stl.h
- include/etl/profiles/segger_gcc_stlport.h
- include/etl/profiles/ticc_no_stl.h
- include/etl/profiles/armv7.h
🧰 Additional context used
📓 Path-based instructions (3)
include/etl/profiles/*.h
⚙️ CodeRabbit configuration file
include/etl/profiles/*.h: Ingnore files in this directory
Files:
include/etl/profiles/determine_builtin_support.h
include/etl/generators/*.h
⚙️ CodeRabbit configuration file
include/etl/generators/*.h: Ingnore files in this directory
Files:
include/etl/generators/type_traits_generator.h
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/deque.hinclude/etl/type_traits.h
🧠 Learnings (3)
📓 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.
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.
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
📚 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/profiles/determine_builtin_support.hinclude/etl/generators/type_traits_generator.hinclude/etl/type_traits.h
🧬 Code graph analysis (2)
include/etl/generators/type_traits_generator.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(195-215)
include/etl/type_traits.h (1)
include/etl/profiles/determine_builtin_support.h (1)
etl(195-215)
🔇 Additional comments (6)
include/etl/deque.h (3)
1761-1763: No behavioural change in push_back.Whitespace-only edits; no functional impact.
Also applies to: 1774-1776
2481-2490: Unconditional repair() assert tightens constraints; confirm intent and comms.ETL_ASSERT(etl::is_trivially_copyable::value, ...) now applies in all configs. This will assert for non-trivially-copyable T even where older builds might have compiled without the check. If intentional, please call this out in release notes/migration guidance.
2482-2486: repair() declaration matches base when ETL_IDEQUE_REPAIR_ENABLE is set.The virtual override aligns with ideque’s pure virtual. Looks good; consider verifying key profiles define ETL_IDEQUE_REPAIR_ENABLE consistently with the base to avoid ODR/API drift.
include/etl/type_traits.h (3)
1949-1961: Builtin gate for is_assignable/is_constructible looks correct.
- is_assignable now uses __is_assignable(T1, T2) under ETL_USING_BUILTIN_IS_ASSIGNABLE.
- is_constructible picks the builtin when available; otherwise falls back to a clean void_t SFINAE helper.
LGTM.
Also applies to: 1963-1971, 1973-1989
2129-2141: Fix for is_trivially_copy_assignable is correct.Using __is_trivially_assignable(T&, const T&) matches the required signature; resolves prior mismatch.
2458-2477: New is_constant_evaluated() backend selection is sensible.C++23 if consteval primary, builtin fallback, else false. Looks good.
2 participants
Description
Currently if we use ETL's no stl default profiles (for example cpp17_no_stl.h), type_traits like is_constructible or is_trivially_copyable doesn't use compiler builtins. Although, the presence of these builtins should be deduced by ETL, which actually it is doing in include/etl/profiles/determine_builtin_support.h, but ETL then is only using the ETL_USE_TYPE_TRAITS_BUILTINS flag.
I believe that ETL should be using either this flag (to force usage of compiler builtins) or to use the other type traits flags that are defined in this file, like for example ETL_USING_BUILTIN_IS_ASSIGNABLE. As such I changed a bit the conditions that are used to define these kind of type traits.
Currently we have the following three conditions:
On the else case each type trait is now wrapped around a condition like this: