Deduce types traits based on if compiler builtins are present #1174
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughReworks Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Preprocessor
participant TypeTraits as include/etl/type_traits.h
participant Compiler
Note over Preprocessor: Evaluate ETL_USING_BUILTIN_* macros
Compiler->>Preprocessor: preprocess translation unit
Preprocessor->>TypeTraits: expand header with per-trait ETL_USING_BUILTIN_* flags
alt Per-trait builtin enabled
TypeTraits->>Compiler: Use compiler intrinsic (e.g., __is_constructible, __is_trivially_destructible)
else Per-trait builtin disabled
TypeTraits->>TypeTraits: Use private_type_traits / void_t / bool_constant fallbacks
end
TypeTraits-->>Compiler: Emit public trait types (same API, different internals)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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.
Comment on lines
2073
to
2117
| #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 | ||
| }; | ||
| #endif | ||
| //********************************************* | ||
| // is_trivially_copy_constructible | ||
| template <typename T> | ||
| struct is_trivially_copy_constructible : public is_trivially_constructible<T, typename add_lvalue_reference<const T>::type> | ||
| { | ||
| }; | ||
| #else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use __is_trivially_constructible; drop GCC __has_trivial_constructor
When the builtin is claimed available, prefer the modern intrinsic (works on GCC/Clang).
-#if ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE
+#if (ETL_USE_TYPE_TRAITS_BUILTINS) || (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<T, void>
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
// is_trivially_copy_constructible unchanged📝 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.
Suggested change
| #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 | |
| }; | |
| #endif | |
| //********************************************* | |
| // is_trivially_copy_constructible | |
| template <typename T> | |
| struct is_trivially_copy_constructible : public is_trivially_constructible<T, typename add_lvalue_reference<const T>::type> | |
| { | |
| }; | |
| #else | |
| #if (ETL_USE_TYPE_TRAITS_BUILTINS) || (ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE) | |
| #if ETL_USING_CPP11 | |
| //********************************************* | |
| // is_trivially_constructible | |
| template <typename T, typename... TArgs> | |
| struct is_trivially_constructible | |
| { | |
| static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs...); | |
| }; | |
| #else | |
| //********************************************* | |
| // is_trivially_constructible | |
| template <typename T, typename TArgs = void> | |
| struct is_trivially_constructible | |
| { | |
| static ETL_CONSTANT bool value = __is_trivially_constructible(T, TArgs); | |
| }; | |
| //********************************************* | |
| // is_trivially_constructible<T, void> | |
| template <typename T> | |
| struct is_trivially_constructible<T, void> | |
| { | |
| static ETL_CONSTANT bool value = __is_trivially_constructible(T); | |
| }; | |
| #endif | |
| //********************************************* | |
| // is_trivially_copy_constructible | |
| template <typename T> | |
| struct is_trivially_copy_constructible | |
| : public is_trivially_constructible<T, typename add_lvalue_reference<const T>::type> | |
| { | |
| }; | |
| #else |
🤖 Prompt for AI Agents
include/etl/type_traits.h lines 2073-2117: the implementations under
ETL_USING_BUILTIN_IS_TRIVIALLY_CONSTRUCTIBLE should use the modern compiler
intrinsic __is_trivially_constructible everywhere instead of the GCC-only
__has_trivial_constructor; update the two/three is_trivially_constructible
specializations (variadic and non-variadic) to call __is_trivially_constructible
with the same template parameter pack (use __is_trivially_constructible(T,
TArgs...) in the C++11 variadic case and __is_trivially_constructible(T, TArgs)
/ __is_trivially_constructible(T) in the non-variadic overloads), and leave
is_trivially_copy_constructible inheriting from the adjusted
is_trivially_constructible as before.
Comment on lines
+2154
to
+2165
| #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.
Comment on lines
+2137
to
+2156
| #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.
Suggested change
| #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? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Summary by CodeRabbit
New Features
Refactor
Compatibility