Convert the macros to templates#1225
Open
VictorSohier wants to merge 12 commits intoRedot-Engine:masterfrom
Open
Convert the macros to templates#1225VictorSohier wants to merge 12 commits intoRedot-Engine:masterfrom
VictorSohier wants to merge 12 commits intoRedot-Engine:masterfrom
Conversation
WalkthroughReorders many VariantUtilityFunctions to take Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![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 (2)
core/variant/variant_utility.cpp (2)
1035-1043:⚠️ Potential issue | 🔴 CriticalMissing early return after error detection.
When
p_arg_count < 1, the error is set but execution continues, callingERR_PRINTwith potentially empty arguments and then overwritingr_error.errorwithCALL_OK. This masks the error from callers.Compare with
str()at lines 954-958 which correctly returns after setting the error.🐛 Proposed fix
void VariantUtilityFunctions::push_error(Callable::CallError &r_error, const Variant **p_args, int p_arg_count) { if (p_arg_count < 1) { r_error.error = Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS; r_error.expected = 1; + return; } ERR_PRINT(join_string(p_args, p_arg_count)); r_error.error = Callable::CallError::CALL_OK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1035 - 1043, The function VariantUtilityFunctions::push_error currently sets r_error.error to CALL_ERROR_TOO_FEW_ARGUMENTS when p_arg_count < 1 but then continues to call ERR_PRINT(join_string(...)) and overwrites r_error.error with CALL_OK; fix this by returning immediately after setting r_error.error and r_error.expected when p_arg_count < 1 so ERR_PRINT and the subsequent assignment to CALL_OK are not executed and the error is preserved for callers (locate the guard in VariantUtilityFunctions::push_error and add an early return similar to the str() implementation).
1045-1053:⚠️ Potential issue | 🔴 CriticalSame missing early return issue in
push_warning.Identical issue as
push_error- error is set but then overwritten withCALL_OK.🐛 Proposed fix
void VariantUtilityFunctions::push_warning(Callable::CallError &r_error, const Variant **p_args, int p_arg_count) { if (p_arg_count < 1) { r_error.error = Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS; r_error.expected = 1; + return; } WARN_PRINT(join_string(p_args, p_arg_count)); r_error.error = Callable::CallError::CALL_OK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1045 - 1053, The push_warning implementation sets r_error.error to CALL_ERROR_TOO_FEW_ARGUMENTS when p_arg_count < 1 but then continues and overwrites it with CALL_OK; update VariantUtilityFunctions::push_warning to return early when p_arg_count < 1 (or otherwise avoid overwriting the error) so the CALL_ERROR_TOO_FEW_ARGUMENTS value is preserved, and only set r_error.error = CALL_OK after successfully handling the warning and printing via WARN_PRINT(join_string(p_args, p_arg_count)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1535-1556: The ptrcall specialization currently forces String
return handling and Variant::STRING via PtrToArg<String>::encode and
get_return_type(), which mis-encodes functions like max/min; change ptrcall to
encode the actual template return type TRet (use PtrToArg<TRet>::encode and
obtain the TRet value from the returned Variant rather than calling operator
String), and update get_return_type() to return the correct Variant::Type for
TRet (implement a small type-trait or separate specializations for TRet==String
and TRet==Variant to map TRet to the proper Variant::Type); update references to
PtrToArg<String>::encode to PtrToArg<TRet>::encode and adjust conversion from
Variant r accordingly in ptrcall.
- Around line 1296-1299: The get_arg_type_helperr overload incorrectly adds +1
to p_arg when forwarding to call_get_argument_type, causing an off-by-one for
functions whose first parameter is Callable::CallError&; change the forwarding
in get_arg_type_helperr (the function template named get_arg_type_helperr that
accepts R (*p_func)(Callable::CallError &, P...), int p_arg) to pass p_arg
directly to call_get_argument_type<P...> (i.e., remove the +1) so the index maps
correctly to the first parameter in the P... pack.
---
Outside diff comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1035-1043: The function VariantUtilityFunctions::push_error
currently sets r_error.error to CALL_ERROR_TOO_FEW_ARGUMENTS when p_arg_count <
1 but then continues to call ERR_PRINT(join_string(...)) and overwrites
r_error.error with CALL_OK; fix this by returning immediately after setting
r_error.error and r_error.expected when p_arg_count < 1 so ERR_PRINT and the
subsequent assignment to CALL_OK are not executed and the error is preserved for
callers (locate the guard in VariantUtilityFunctions::push_error and add an
early return similar to the str() implementation).
- Around line 1045-1053: The push_warning implementation sets r_error.error to
CALL_ERROR_TOO_FEW_ARGUMENTS when p_arg_count < 1 but then continues and
overwrites it with CALL_OK; update VariantUtilityFunctions::push_warning to
return early when p_arg_count < 1 (or otherwise avoid overwriting the error) so
the CALL_ERROR_TOO_FEW_ARGUMENTS value is preserved, and only set r_error.error
= CALL_OK after successfully handling the warning and printing via
WARN_PRINT(join_string(p_args, p_arg_count)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5886422a-3c32-4343-8615-3e31a4b50d98
📒 Files selected for processing (3)
core/variant/method_ptrcall.hcore/variant/variant_utility.cppcore/variant/variant_utility.h
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/variant/variant_utility.cpp (2)
1298-1301:⚠️ Potential issue | 🟠 MajorRemove the
+1offset in CallError argument-type lookup.
P...already excludesCallable::CallError &, so shiftingp_argmisreports argument types for allR(Callable::CallError &, P...)registrations.Proposed fix
template <typename R, typename... P> static _FORCE_INLINE_ Variant::Type get_arg_type_helperr(R (*p_func)(Callable::CallError &, P...), int p_arg) { - return call_get_argument_type<P...>(p_arg + 1); + return call_get_argument_type<P...>(p_arg); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1298 - 1301, The get_arg_type_helperr helper incorrectly adds a +1 offset when calling call_get_argument_type, causing arg indices to be shifted for functions of signature R (*p_func)(Callable::CallError &, P...); update get_arg_type_helperr to call call_get_argument_type<P...>(p_arg) (i.e., remove the +1) so the argument-type lookup for the parameter pack P... matches the original p_arg value.
1548-1548:⚠️ Potential issue | 🔴 CriticalGeneric vararg-return binder is incorrectly hardcoded to
String.This specialization is templated on
TRet, butptrcallandget_return_type()forceString, which breaks non-Stringfunctions (e.g.,max/minreturningVariant).Proposed fix
static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); - PtrToArg<String>::encode(r.operator String(), ret); + PtrToArg<TRet>::encode(r.operator TRet(), ret); } @@ static Variant::Type get_return_type() { - return Variant::STRING; + return GetTypeInfo<TRet>::VARIANT_TYPE; }Also applies to: 1556-1558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` at line 1548, The vararg-return binder specialization currently hardcodes String by calling PtrToArg<String>::encode and forcing get_return_type()/ptrcall to String; update the specialization to use the template parameter TRet instead of String (e.g., replace PtrToArg<String>::encode(...) with PtrToArg<TRet>::encode(...) and ensure ptrcall/get_return_type return/type-check against TRet), and apply the same change to the other identical occurrences in the file (the block around the PtrToArg usage at the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1298-1301: The get_arg_type_helperr helper incorrectly adds a +1
offset when calling call_get_argument_type, causing arg indices to be shifted
for functions of signature R (*p_func)(Callable::CallError &, P...); update
get_arg_type_helperr to call call_get_argument_type<P...>(p_arg) (i.e., remove
the +1) so the argument-type lookup for the parameter pack P... matches the
original p_arg value.
- Line 1548: The vararg-return binder specialization currently hardcodes String
by calling PtrToArg<String>::encode and forcing get_return_type()/ptrcall to
String; update the specialization to use the template parameter TRet instead of
String (e.g., replace PtrToArg<String>::encode(...) with
PtrToArg<TRet>::encode(...) and ensure ptrcall/get_return_type return/type-check
against TRet), and apply the same change to the other identical occurrences in
the file (the block around the PtrToArg usage at the other occurrence).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d0e3b52-d9f7-42ab-894b-b7caa1ccea48
📒 Files selected for processing (1)
core/variant/variant_utility.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/variant/variant_utility.cpp (1)
1586-1606:⚠️ Potential issue | 🔴 CriticalCritical:
ptrcalldoesn't encode the result for generic TRet vararg functions.The
ptrcallimplementation creates a localVariant r, callsvalidated_call, but then discards the result without encoding it toret. This affectsmaxandminfunctions which returnVariant.GDExtension consumers calling these functions via
get_ptr_utility_function()will receive uninitialized/garbage data since the result is never written.Additionally,
get_return_type()returnsVariant::NILunconditionally instead of using the actual template type.🐛 Proposed fix
static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); + PtrToArg<TRet>::encode(r, ret); } // ... static Variant::Type get_return_type() { - return Variant::NIL; + return GetTypeInfo<TRet>::VARIANT_TYPE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1586 - 1606, ptrcall currently calls validated_call(&r, ...) then drops r instead of encoding it into the void* ret buffer; update ptrcall (in the same template struct containing PtrToArg and validated_call) to encode the resulting Variant r into the ret pointer using the same encoding/path used elsewhere for TRet (i.e., write the proper TRet representation into ret via the variant-to-pointer conversion used for PtrToArg), and change get_return_type() to return the Variant::Type corresponding to the template return type (use the existing mapping/trait that converts TRet to Variant::Type) instead of always Variant::NIL so callers receive the correct return-type metadata.
🧹 Nitpick comments (1)
core/variant/variant_utility.cpp (1)
1318-1345: Remove commented-out dead code.This block contains experimental code that was not used. Consider removing it to reduce maintenance burden. If needed for future reference, it's preserved in version control history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1318 - 1345, Remove the commented-out experimental dead code block containing CallHelperVoid, CallHelperRet, the _call_helper template and the call_helperp2 function stub from variant_utility.cpp; locate the commented region around the identifiers CallHelperVoid, CallHelperRet, _call_helper and call_helperp2 and delete those commented lines so the file no longer contains unused commented code (rely on VCS history if the code needs to be recovered).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1586-1606: ptrcall currently calls validated_call(&r, ...) then
drops r instead of encoding it into the void* ret buffer; update ptrcall (in the
same template struct containing PtrToArg and validated_call) to encode the
resulting Variant r into the ret pointer using the same encoding/path used
elsewhere for TRet (i.e., write the proper TRet representation into ret via the
variant-to-pointer conversion used for PtrToArg), and change get_return_type()
to return the Variant::Type corresponding to the template return type (use the
existing mapping/trait that converts TRet to Variant::Type) instead of always
Variant::NIL so callers receive the correct return-type metadata.
---
Nitpick comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1318-1345: Remove the commented-out experimental dead code block
containing CallHelperVoid, CallHelperRet, the _call_helper template and the
call_helperp2 function stub from variant_utility.cpp; locate the commented
region around the identifiers CallHelperVoid, CallHelperRet, _call_helper and
call_helperp2 and delete those commented lines so the file no longer contains
unused commented code (rely on VCS history if the code needs to be recovered).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07273228-795b-4a0a-b258-849c2034bc36
📒 Files selected for processing (1)
core/variant/variant_utility.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/variant/variant_utility.cpp (1)
1558-1569:⚠️ Potential issue | 🔴 Critical
ptrcall()never writes the vararg return back to the caller.Lines 1721-1726 bind
max()andmin()through this specialization.validated_call()fills the localVariant r, but nothing ever encodesrintoret, so PTR callers get stale/undefined output.🐛 Proposed fix
static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); + PtrToArg<TRet>::encode(VariantCaster<TRet>::cast(r), ret); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1558 - 1569, ptrcall builds argument Variants and calls validated_call(&r,...), but never writes the result back to the caller pointer ret; update ptrcall to encode the local Variant r into the caller buffer after validated_call returns (e.g. call the corresponding pointer-write helper such as ArgToPtr<Variant>::write or a Ptr/Arg conversion routine with ret and r) so the PTR ABI receives the computed value; keep existing usage of PtrToArg<Variant>::convert for inputs and add the symmetric call (using the project's Variant-to-pointer helper) to store r into ret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1473-1477: The current vararg utility wrapper hardcodes
get_argument_count() to 1 and returns Variant::NIL from get_argument_type(),
which misreports minimum arity for bound utilities; update the wrapper(s) so
get_argument_count() returns the actual minimum arity for each registered
utility (e.g., for the generic-return vararg wrapper used by print* set min
arity to 0, and for max/min set min arity to 2) and ensure get_argument_type(int
p_arg) still reports the correct per-argument type; apply the same change in
both locations (the pair around the generic-return vararg wrapper and the
similar block at the other occurrence) so
Variant::get_utility_function_argument_count() and the signature hash reflect
the true arities.
- Around line 1634-1791: The calls use the nonstandard GNU extension typeof(...)
(e.g. in lines registering VariantUtilityFunctions::sin, ::cos, ::tan, etc.),
which breaks MSVC; replace each Func<typeof(VariantUtilityFunctions::X)> with
Func<decltype(&VariantUtilityFunctions::X)> so the type deduction is standard
C++11, keeping the existing FuncInner<...,
Variant::UTILITY_FUNC_TYPE_...>::register_fn(...) invocations unchanged; update
every occurrence in this block (functions like VariantUtilityFunctions::sin,
::atan2, ::randi_range, ::weakref, ::type_convert, etc.) to use decltype(&...)
instead of typeof(...).
---
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1558-1569: ptrcall builds argument Variants and calls
validated_call(&r,...), but never writes the result back to the caller pointer
ret; update ptrcall to encode the local Variant r into the caller buffer after
validated_call returns (e.g. call the corresponding pointer-write helper such as
ArgToPtr<Variant>::write or a Ptr/Arg conversion routine with ret and r) so the
PTR ABI receives the computed value; keep existing usage of
PtrToArg<Variant>::convert for inputs and add the symmetric call (using the
project's Variant-to-pointer helper) to store r into ret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41d77028-4542-498b-867c-adef80ebd9d1
📒 Files selected for processing (1)
core/variant/variant_utility.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/variant/variant_utility.cpp (1)
1558-1569:⚠️ Potential issue | 🔴 Critical
ptrcalldoes not encode the return value—max/minwill return uninitialized data.The result is computed into local
Variant rat line 1568, but the function returns without writing anything to theretpointer. Compare with the String specialization (lines 1519-1520) which correctly encodes the result.🐛 Proposed fix
static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); + PtrToArg<Variant>::encode(r, ret); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1558 - 1569, The ptrcall helper currently computes the result into a local Variant r but never writes it to the caller-provided ret pointer, causing uninitialized returns for functions like max/min; modify ptrcall (the function that constructs args with PtrToArg<Variant>::convert, builds argsp, calls validated_call(&r, ...)) to encode/copy the result Variant r into the void* ret using the same conversion/encoding used by the String specialization — i.e., after validated_call returns, write the encoded Variant result into ret (using the appropriate PtrFromRet or equivalent conversion used elsewhere) so the caller receives the computed value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1558-1569: The ptrcall helper currently computes the result into a
local Variant r but never writes it to the caller-provided ret pointer, causing
uninitialized returns for functions like max/min; modify ptrcall (the function
that constructs args with PtrToArg<Variant>::convert, builds argsp, calls
validated_call(&r, ...)) to encode/copy the result Variant r into the void* ret
using the same conversion/encoding used by the String specialization — i.e.,
after validated_call returns, write the encoded Variant result into ret (using
the appropriate PtrFromRet or equivalent conversion used elsewhere) so the
caller receives the computed value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a143a94e-dce4-4f4e-a7c5-bfe2f26bd045
📒 Files selected for processing (1)
core/variant/variant_utility.cpp
Contributor
|
@VictorSohier Changing the interface that users interact with can be risky. Do we know if moving around the parameters of these functions will effect user projects? As in are these functions exposed to users? |
Author
The error parameter isn't exposed in GDScript and these functions aren't called from anywhere else. All calls to these functions happen through the call helpers. Still compiles and tests still pass. Maybe for paranoia's sake, I should find a test project and see if it still functions. Ultimately though, this is a limitation of template specialization. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/variant/variant_utility.cpp (1)
1545-1556:⚠️ Potential issue | 🔴 Critical
ptrcalldrops the computed return value for generic vararg bindings.At Line 1545-1556,
validated_callwrites into localVariant r, butretis never encoded. This breaks ptrcall return propagation for this specialization (notablyVariant-returning vararg utilities).💡 Proposed fix
static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); + PtrToArg<TRet>::encode(static_cast<TRet>(r), ret); }#!/bin/bash # Confirm ptrcall in generic vararg specialization does not encode ret. rg -n -C6 'struct Func<TRet\(Callable::CallError &, const Variant \*\*, int\)>' core/variant/variant_utility.cpp rg -n -C4 'validated_call\(&r, \(const Variant \*\*\)argsp\.ptr\(\), p_argcount\);' core/variant/variant_utility.cpp rg -n 'PtrToArg<TRet>::encode' core/variant/variant_utility.cpp # Show current registrations that likely hit this specialization (e.g., max/min). rg -n 'bind_fn_vuf\((max|min),' core/variant/variant_utility.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1545 - 1556, ptrcall currently calls validated_call(&r, ...) but never writes the computed Variant r into the provided ret pointer; update ptrcall to encode the local r back into ret using the PtrToArg<Variant>::encode routine (or the generic PtrToArg<TRet>::encode when templated) after validated_call returns so the return value is propagated; reference symbols: ptrcall, validated_call, Variant r, ret, and PtrToArg<Variant>::encode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1375-1378: validated_call_helper currently does *ret = p_func(...)
which is invalid when p_func returns void; change the implementation to detect
the call return type (e.g. using std::invoke_result_t or decltype on p_func) and
use if constexpr to handle void vs non-void: if the return type is void, call
p_func(err, ...) and then set *ret to a default/empty Variant (or otherwise
initialize it); otherwise call and assign the returned value to *ret. Reference
validated_call_helper, p_func, Callable::CallError and Variant *ret when
applying the conditional fix.
---
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1545-1556: ptrcall currently calls validated_call(&r, ...) but
never writes the computed Variant r into the provided ret pointer; update
ptrcall to encode the local r back into ret using the PtrToArg<Variant>::encode
routine (or the generic PtrToArg<TRet>::encode when templated) after
validated_call returns so the return value is propagated; reference symbols:
ptrcall, validated_call, Variant r, ret, and PtrToArg<Variant>::encode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce671111-0682-4c3b-a5c7-2b577c3dc9c7
📒 Files selected for processing (2)
core/variant/type_info.hcore/variant/variant_utility.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/variant/variant_utility.cpp (1)
35-42:⚠️ Potential issue | 🟡 MinorRemove duplicate include of
binder_common.h.Line 35 includes
"binder_common.h"(relative path) and line 42 includes"core/variant/binder_common.h"(full path). Both resolve to the same file. Remove one to avoid redundancy.Suggested fix
`#include` "variant_utility.h" -#include "binder_common.h" `#include` "core/io/marshalls.h" `#include` "core/object/ref_counted.h" `#include` "core/os/os.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 35 - 42, The file contains a duplicate include of binder_common.h; remove one of the two includes so the header is only included once (e.g., delete the duplicate relative include "binder_common.h" or the duplicate "core/variant/binder_common.h" line), leaving a single include for binder_common.h to avoid redundancy and potential maintenance confusion—update the includes near the top of variant_utility.cpp to keep only one binder_common.h reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1545-1556: The ptrcall function builds the result into local
Variant r but never writes it to the output pointer ret; update ptrcall to
encode or placement-construct the result into ret (similar to the String
specialization): use PtrToArg<Variant>::encode(r, ret) if PtrToArg<Variant>
provides encode, otherwise call memnew_placement(ret, Variant(r)) to construct
the returned Variant in-place; modify the ptrcall function (which creates args,
argsp and Variant r) to perform this encode/placement before returning.
---
Outside diff comments:
In `@core/variant/variant_utility.cpp`:
- Around line 35-42: The file contains a duplicate include of binder_common.h;
remove one of the two includes so the header is only included once (e.g., delete
the duplicate relative include "binder_common.h" or the duplicate
"core/variant/binder_common.h" line), leaving a single include for
binder_common.h to avoid redundancy and potential maintenance confusion—update
the includes near the top of variant_utility.cpp to keep only one
binder_common.h reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2119408-8df4-4b29-b0c3-ffd5fa22be05
📒 Files selected for processing (2)
core/variant/type_info.hcore/variant/variant_utility.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/variant/variant_utility.cpp (2)
1545-1556:⚠️ Potential issue | 🟠 MajorThe generic vararg
ptrcallnever stores its computed return value.Line 1555 materializes the result in
Variant r, but nothing encodes it intoret.max()andmin()use this wrapper, so PTR utility callers can observe an uninitialized or stale return slot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1545 - 1556, The ptrcall wrapper computes the return in local Variant r but never writes it back to the provided ret slot; update ptrcall (the function that calls validated_call and declares Variant r) to store the result into ret after calling validated_call (for example, copy or assign r into the memory pointed to by ret, e.g. *(Variant*)ret = r) so callers (e.g., max/min via the PTR utility) observe the computed return value.
1374-1378:⚠️ Potential issue | 🟠 Major
validated_call_helpercannot assign avoidresult.Line 1377 does
*ret = p_func(...)in thevoid(Callable::CallError &, TArgs...)specialization. That is ill-formed C++, so the first non-varargvoidutility registered through this helper will fail to compile.Suggested fix
template <size_t... Is> static _FORCE_INLINE_ void validated_call_helper(void (*p_func)(Callable::CallError &, TArgs...), Variant *ret, const Variant **p_args, IndexSequence<Is...>) { Callable::CallError err; - *ret = p_func(err, VariantCaster<TArgs>::cast(*p_args[Is])...); + p_func(err, VariantCaster<TArgs>::cast(*p_args[Is])...); + (void)ret; (void)p_args; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1374 - 1378, validated_call_helper currently attempts to assign the result of a void function to *ret (using *ret = p_func(...)), which is ill-formed; change the body for the void-returning specialization (the template with void (*p_func)(Callable::CallError &, TArgs...)) to call p_func(err, VariantCaster<TArgs>::cast(*p_args[Is])...) without assignment and then set *ret to a default/empty Variant (e.g., *ret = Variant()) so the caller still gets a valid Variant; reference the function validated_call_helper, the parameter p_func, the types Callable::CallError and VariantCaster<TArgs>, and the pointer Variant *ret when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1283-1286: The call_helper function currently reuses the same
Callable::CallError (r_error) that VariantCasterAndValidate writes into, which
lets a subsequent assignment r_error.error = CALL_OK erase caster-side failures;
modify call_helper (and the same pattern at the other occurrence around the
1360-1363 region) to validate into a local temporary Callable::CallError
temp_error and, if temp_error.error != CALL_OK, copy that error into r_error and
return before calling p_func/VCALLER; only if temp_error is OK proceed to invoke
p_func and set r_error based on the callee result.
---
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1545-1556: The ptrcall wrapper computes the return in local
Variant r but never writes it back to the provided ret slot; update ptrcall (the
function that calls validated_call and declares Variant r) to store the result
into ret after calling validated_call (for example, copy or assign r into the
memory pointed to by ret, e.g. *(Variant*)ret = r) so callers (e.g., max/min via
the PTR utility) observe the computed return value.
- Around line 1374-1378: validated_call_helper currently attempts to assign the
result of a void function to *ret (using *ret = p_func(...)), which is
ill-formed; change the body for the void-returning specialization (the template
with void (*p_func)(Callable::CallError &, TArgs...)) to call p_func(err,
VariantCaster<TArgs>::cast(*p_args[Is])...) without assignment and then set *ret
to a default/empty Variant (e.g., *ret = Variant()) so the caller still gets a
valid Variant; reference the function validated_call_helper, the parameter
p_func, the types Callable::CallError and VariantCaster<TArgs>, and the pointer
Variant *ret when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f4bde93-27dc-4bdb-a0c6-97463b784052
📒 Files selected for processing (3)
core/variant/type_info.hcore/variant/variant_utility.cppdoc/classes/@GlobalScope.xml
Comment on lines
+1283
to
+1286
| static _FORCE_INLINE_ void call_helper(TRet (*p_func)(Callable::CallError &, TArgs...), Variant *ret, const Variant **p_args, Callable::CallError &r_error, IndexSequence<Is...>) { | ||
| r_error.error = Callable::CallError::CALL_OK; | ||
| VCALLER(TArgs, ret, p_args, Is, r_error); | ||
| (void)p_args; // avoid gcc warning |
There was a problem hiding this comment.
Don’t let the callee erase binder-side cast failures.
Under DEBUG_ENABLED, VariantCasterAndValidate(...) writes into r_error before p_func(...) runs. Reusing that same object for the callee means any later CALL_OK assignment inside the utility can clear a conversion error. lerp() is affected today: an invalid weight can be default-cast and still return success. Validate into a temporary Callable::CallError, or bail out before invoking p_func once casting fails.
Also applies to: 1360-1363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/variant/variant_utility.cpp` around lines 1283 - 1286, The call_helper
function currently reuses the same Callable::CallError (r_error) that
VariantCasterAndValidate writes into, which lets a subsequent assignment
r_error.error = CALL_OK erase caster-side failures; modify call_helper (and the
same pattern at the other occurrence around the 1360-1363 region) to validate
into a local temporary Callable::CallError temp_error and, if temp_error.error
!= CALL_OK, copy that error into r_error and return before calling
p_func/VCALLER; only if temp_error is OK proceed to invoke p_func and set
r_error based on the callee result.
Author
There was a problem hiding this comment.
This is identical behavior as to before the change: https://github.com/Redot-Engine/redot-engine/pull/1225/changes/BASE..e918047c53dfecfc732a9596ecf4a609d622577e#diff-423b77401174ecabdadd4c011ae8bb4c2cbc205bca2966a3a1bb00116680549dL1212-L1218
Just that before, there wasn't an explicit macro to handle this case.
There was a problem hiding this comment.
@VictorSohier, you're right — looking at the original call_helperpr, it already set r_error.error = CALL_OK upfront and passed the same r_error into the VCALLR expansion, which is the same pattern as the new code. This is a pre-existing behavior that predates this PR and is out of scope here. Thanks for the clarification!
✏️ Learnings added
Learnt from: VictorSohier
Repo: Redot-Engine/redot-engine PR: 1225
File: core/variant/variant_utility.cpp:1283-1286
Timestamp: 2026-03-17T01:20:15.454Z
Learning: In core/variant/variant_utility.cpp (Redot Engine), the pattern of setting r_error.error = CALL_OK before invoking VariantCasterAndValidate with the same r_error object in call_helper/call_helperpr is pre-existing behavior from before PR `#1225`. Do not flag this pattern as a regression introduced by the macro-to-template refactor; it is identical to the original implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: VictorSohier
Repo: Redot-Engine/redot-engine PR: 1225
File: core/variant/variant_utility.cpp:1473-1477
Timestamp: 2026-03-14T02:00:02.021Z
Learning: In core/variant/variant_utility.cpp (Redot Engine), the get_argument_count() returning 1 for vararg utility function specializations (e.g., Func<void(Callable::CallError &, const Variant **, int)>) is intentional and harmless. The argcount validation in Variant::call_utility_function() is fully gated behind !bfi->is_vararg, so the stored argcount is never used for dispatch validation on vararg functions. Minimum arity for vararg functions is enforced inside each function body. Do not flag these vararg cases for arity issues; ensure review notes focus on non-vararg dispatch paths and per-function arity checks instead.
This likely would've been fixed if and when this case was realized, however, it was incorrect by construction and probably should be handled now.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/variant/variant_utility.cpp (1)
1546-1557:⚠️ Potential issue | 🟠 MajorAdd result encoding to generic vararg
ptrcall.The generic vararg
ptrcallimplementation (lines 1546–1557) computes the result into localVariant rbut never encodes it to the outputretpointer. The String specialization at line 1520 correctly usesPtrToArg<String>::encode()to write the result; the generic template should follow the same pattern.static void ptrcall(void *ret, const void **p_args, int p_argcount) { Vector<Variant> args; for (int i = 0; i < p_argcount; i++) { args.push_back(PtrToArg<Variant>::convert(p_args[i])); } Vector<const Variant *> argsp; for (int i = 0; i < p_argcount; i++) { argsp.push_back(&args[i]); } Variant r; validated_call(&r, (const Variant **)argsp.ptr(), p_argcount); + PtrToArg<Variant>::encode(r, ret); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 1546 - 1557, The ptrcall function currently computes the call result into local Variant r but never writes it back to the output pointer ret; modify ptrcall to encode the result into ret after validated_call by calling the appropriate encoder (use PtrToArg<Variant>::encode(&r, ret) or the same encoding pattern used by the String specialization), leaving the existing argument conversion (PtrToArg<Variant>::convert) and validated_call invocation unchanged.
🧹 Nitpick comments (1)
core/variant/variant_utility.cpp (1)
35-42: Duplicate include ofbinder_common.h.Line 35 includes
"binder_common.h"(relative path) and line 42 includes"core/variant/binder_common.h"(full path). These resolve to the same file. Remove one of them.`#include` "variant_utility.h" -#include "binder_common.h" `#include` "core/io/marshalls.h" `#include` "core/object/ref_counted.h" `#include` "core/os/os.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant_utility.cpp` around lines 35 - 42, Remove the duplicate include of binder_common.h: there are two `#include` directives for the same header (one as "binder_common.h" and one as "core/variant/binder_common.h"); delete the redundant one (keep the correctly scoped include "core/variant/binder_common.h" to maintain consistent include paths) so only a single include for binder_common.h remains in variant_utility.cpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/variant/variant_utility.cpp`:
- Around line 1546-1557: The ptrcall function currently computes the call result
into local Variant r but never writes it back to the output pointer ret; modify
ptrcall to encode the result into ret after validated_call by calling the
appropriate encoder (use PtrToArg<Variant>::encode(&r, ret) or the same encoding
pattern used by the String specialization), leaving the existing argument
conversion (PtrToArg<Variant>::convert) and validated_call invocation unchanged.
---
Nitpick comments:
In `@core/variant/variant_utility.cpp`:
- Around line 35-42: Remove the duplicate include of binder_common.h: there are
two `#include` directives for the same header (one as "binder_common.h" and one as
"core/variant/binder_common.h"); delete the redundant one (keep the correctly
scoped include "core/variant/binder_common.h" to maintain consistent include
paths) so only a single include for binder_common.h remains in
variant_utility.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2f23296-6ce4-4a5b-baf9-6ac2172d69b4
📒 Files selected for processing (1)
core/variant/variant_utility.cpp
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This has been discussed for a hot minute moving these to templates from C macros.
A consequence of this is that any functions which emit an error as a value now must have their error parameter be the first parameter.
Other than that, it should now be less cumbersome to manage, be better parameterizable and more flexible, namely, being able to register functions that aren't tied to the
VariantUtilityFunctionsstruct and actually being able to use function overloading and templated functions as a result. Also, the registered name is now dis-associated from the function name provided.Summary by CodeRabbit
Refactor
Bug Fixes
Documentation