Reduce cases, branches, etc, with regard to Variant#1222
Open
VictorSohier wants to merge 13 commits intoRedot-Engine:masterfrom
Open
Reduce cases, branches, etc, with regard to Variant#1222VictorSohier wants to merge 13 commits intoRedot-Engine:masterfrom
VictorSohier wants to merge 13 commits intoRedot-Engine:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced per-type branching in Variant with table-driven structures for type names, casts, strict casts, destructors, and bucket management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.cpp`:
- Around line 2807-2822: Variant::_variant_call_error is missing messages for
several Callable::CallError cases so err can be empty; add explicit case arms
for Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS,
::CALL_ERROR_INSTANCE_IS_NULL, and ::CALL_ERROR_METHOD_NOT_CONST (and update the
default to produce a generic message) and set err to descriptive messages that
include p_method and Variant::get_type_name(type) or argument/index info as
appropriate so ERR_PRINT() never prints a blank line.
- Around line 453-455: Variant::is_null currently treats every non-OBJECT
variant as null because it negates (type == OBJECT && _get_obj().obj); change it
so only genuinely null/uninitialized variants or OBJECTs with a null internal
pointer are considered null: in Variant::is_null(), if type == OBJECT return
!_get_obj().obj, else if the variant represents the explicit
null/none/uninitialized kind return true, otherwise return false; update logic
referencing Variant::is_null, the type enum values (the explicit null/none kind)
and _get_obj() to implement this precise branching.
- Around line 720-730: The cleanup helpers are being called through incompatible
function pointer types (destroy<T> and bucketFree<T> are cast to Destructor and
BucketFree), which is UB; replace the casts with proper type-safe adapter
functions and use those adapters for the stored function pointers. Add
non-templated (or templated-but-instantiated) adapter functions such as
destroyAdapter<T>(void* p) { reinterpret_cast<T*>(p)->~T(); } and
bucketFreeAdapter<T>(void* bucket, void* mem) { static_cast<PagedAllocator<T,
true>*>(bucket)->free(static_cast<T*>(mem)); } and assign pointers to these
adapter functions to Destructor and BucketFree instead of casting the original
templated functions; refer to destroy, bucketFree, Destructor, BucketFree and
PagedAllocator<T, true> when making the change.
In `@core/variant/variant.h`:
- Around line 283-305: The needs_deinit bitmask is missing Variant::RID so
_clear_internal()'s RID destructor path is skipped; add (1ull << Variant::RID)
into the static constexpr std::uint64_t needs_deinit mask so that RID cleanup is
performed when clear() or ~Variant() route teardown through that mask (update
the mask near the declaration of needs_deinit in variant.h to include
Variant::RID).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0332c04f-160b-40bc-8223-122db4d6087b
📒 Files selected for processing (2)
core/variant/variant.cppcore/variant/variant.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
core/variant/variant.h (1)
283-305:⚠️ Potential issue | 🟠 MajorRestore
Variant::RIDinneeds_deinit.
_clear_internal()still has aRIDteardown path, butclear()/~Variant()now route all cleanup through this mask. LeavingRIDout skipsRIDdestruction entirely.Suggested fix
static constexpr std::uint64_t needs_deinit = (1ull << Variant::STRING) | (1ull << Variant::TRANSFORM2D) | (1ull << Variant::AABB) | (1ull << Variant::BASIS) | (1ull << Variant::TRANSFORM3D) | (1ull << Variant::PROJECTION) | (1ull << Variant::STRING_NAME) | (1ull << Variant::NODE_PATH) | + (1ull << Variant::RID) | (1ull << Variant::OBJECT) | (1ull << Variant::CALLABLE) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.h` around lines 283 - 305, The needs_deinit mask omits Variant::RID causing RID cleanup to be skipped when clear() / ~Variant() route teardown through this mask; add (1ull << Variant::RID) back into the static constexpr std::uint64_t needs_deinit definition so _clear_internal()'s RID teardown path is reached during Variant destruction/clear, ensuring Variant::RID is included alongside the other types in needs_deinit.core/variant/variant.cpp (3)
2859-2874:⚠️ Potential issue | 🟡 MinorPopulate the remaining
CallErrorbranches here.
CALL_ERROR_TOO_FEW_ARGUMENTS,CALL_ERROR_INSTANCE_IS_NULL, andCALL_ERROR_METHOD_NOT_CONSTstill leaveerrempty, soERR_PRINT()emits a blank line for those failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 2859 - 2874, The error-handling switch in Variant::_variant_call_error leaves several Callable::CallError branches empty; implement messages for CALL_ERROR_TOO_FEW_ARGUMENTS (e.g., "Too few arguments for method 'p_method', expected at least X, got Y" using error.argument/expected), CALL_ERROR_INSTANCE_IS_NULL (e.g., "Instance is null for method 'p_method' on type 'Variant::get_type_name(type)'"), and CALL_ERROR_METHOD_NOT_CONST (e.g., "Method 'p_method' is not const and cannot be called on a const instance"); ensure you use the existing helpers itos(), Variant::get_type_name(), p_method and type, populate err in each branch, and keep the default branch producing a sensible fallback message before calling ERR_PRINT(err.utf8().get_data()).
450-452:⚠️ Potential issue | 🔴 Critical
is_null()now treats every non-OBJECTvalue as null.This expression only returns
falsefor a liveOBJECT.INT,STRING,ARRAY, etc. all become “null”, which is a breaking semantic change.Suggested fix
bool Variant::is_null() const { - return !(type == OBJECT && _get_obj().obj); + return type == NIL || (type == OBJECT && _get_obj().obj == nullptr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 450 - 452, Variant::is_null currently treats every non-OBJECT type as null; update its logic so it only reports null for the explicit null variant type or for an OBJECT whose internal pointer is null. Specifically, change Variant::is_null to check the variant's null discriminator (e.g., the explicit NULL/NIL enum value) OR (type == OBJECT && !_get_obj().obj), instead of negating (type == OBJECT && _get_obj().obj); use the existing type enum and _get_obj().obj accessor to implement this.
718-729:⚠️ Potential issue | 🟠 MajorUse type-safe adapters in the destructor tables.
destroy<T>andbucketFree<T>are cast to unrelated function pointer types and then invoked throughDestructor/BucketFree. That is undefined behavior in C++ and makes teardown ABI-dependent.Suggested fix
-template <typename T> -void destroy(T *s) { - reinterpret_cast<T *>(s)->~T(); +template <typename T> +static void destroy_adapter(void *p_mem) { + static_cast<T *>(p_mem)->~T(); } template <typename T> -void bucketFree(PagedAllocator<T, true> *bucket, void *mem) { - ((PagedAllocator<T, true> *)bucket)->free((T *)mem); +static void bucket_free_adapter(void *p_bucket, void *p_mem) { + static_cast<PagedAllocator<T, true> *>(p_bucket)->free(static_cast<T *>(p_mem)); } @@ - (Destructor)destroy<String>, + destroy_adapter<String>, @@ - (Destructor)destroy<Transform2D>, + destroy_adapter<Transform2D>, @@ - (Destructor)destroy<::AABB>, - (Destructor)destroy<Basis>, - (Destructor)destroy<Transform3D>, - (Destructor)destroy<Projection>, + destroy_adapter<::AABB>, + destroy_adapter<Basis>, + destroy_adapter<Transform3D>, + destroy_adapter<Projection>, @@ - (Destructor)destroy<StringName>, - (Destructor)destroy<NodePath>, - (Destructor)destroy<::RID>, + destroy_adapter<StringName>, + destroy_adapter<NodePath>, + destroy_adapter<::RID>, @@ - (Destructor)destroy<Callable>, - (Destructor)destroy<Signal>, - (Destructor)destroy<Dictionary>, - (Destructor)destroy<Array>, + destroy_adapter<Callable>, + destroy_adapter<Signal>, + destroy_adapter<Dictionary>, + destroy_adapter<Array>, @@ - (BucketFree)bucketFree<Variant::Pools::BucketSmall>, + bucket_free_adapter<Variant::Pools::BucketSmall>, @@ - (BucketFree)bucketFree<Variant::Pools::BucketSmall>, - (BucketFree)bucketFree<Variant::Pools::BucketMedium>, - (BucketFree)bucketFree<Variant::Pools::BucketMedium>, - (BucketFree)bucketFree<Variant::Pools::BucketLarge>, + bucket_free_adapter<Variant::Pools::BucketSmall>, + bucket_free_adapter<Variant::Pools::BucketMedium>, + bucket_free_adapter<Variant::Pools::BucketMedium>, + bucket_free_adapter<Variant::Pools::BucketLarge>,#!/bin/bash # Verify the helper signatures and the casted entries stored in the cleanup dispatch tables. sed -n '718,845p' core/variant/variant.cppAlso applies to: 732-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 718 - 729, The destructor tables store pointers to destroy<T> and bucketFree<T> via reinterpret_cast to the generic typedefs Destructor and BucketFree which is UB; replace those casts with type-safe adapter functions that have the exact signatures used in the tables (e.g., a non-template void destroy_adapter(void *p) { static_cast<T*>(p)->~T(); } and void bucket_free_adapter(void *bucket, void *mem) { static_cast<PagedAllocator<T, true>*>(bucket)->free(static_cast<T*>(mem)); }) and store pointers to those adapters instead of casting destroy<T>/bucketFree<T>; update all places referencing Destructor and BucketFree to expect the adapter signatures and remove the reinterpret_casts involving destroy<T> and bucketFree<T>.
🤖 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.cpp`:
- Around line 2836-2841: IS_SHARED currently omits packed array types so
Variant::is_type_shared/ is_shared returns false for PACKED_*_ARRAYs; update the
IS_SHARED bitmask to include all packed-array Variant enum values by OR-ing
(1ull << Variant::PACKED_..._ARRAY) for each PACKED_*_ARRAY variant (i.e., add
the PACKED array enum entries to the existing mask) so
Variant::is_type_shared(Variant::Type p_type) correctly reports them as shared.
- Around line 202-205: The code indexes TYPE_STRING_TABLE and uses bit shifts
with Variant::Type without checking for negative values; update
Variant::get_type_name (and the similar helper in lines 221-234) to validate
that p_type is within the full valid range (p_type >= 0 && p_type < VARIANT_MAX)
before indexing or shifting, returning a safe fallback string (e.g., "Invalid")
or handling out-of-range values; additionally, at the call sites where signed
ints are cast to Variant::Type (the places using error.expected and
ce.expected), validate the signed int is within [0, VARIANT_MAX-1] before
casting or pass a sanitized/validated value to the helper to prevent negative
indices and UB.
---
Duplicate comments:
In `@core/variant/variant.cpp`:
- Around line 2859-2874: The error-handling switch in
Variant::_variant_call_error leaves several Callable::CallError branches empty;
implement messages for CALL_ERROR_TOO_FEW_ARGUMENTS (e.g., "Too few arguments
for method 'p_method', expected at least X, got Y" using
error.argument/expected), CALL_ERROR_INSTANCE_IS_NULL (e.g., "Instance is null
for method 'p_method' on type 'Variant::get_type_name(type)'"), and
CALL_ERROR_METHOD_NOT_CONST (e.g., "Method 'p_method' is not const and cannot be
called on a const instance"); ensure you use the existing helpers itos(),
Variant::get_type_name(), p_method and type, populate err in each branch, and
keep the default branch producing a sensible fallback message before calling
ERR_PRINT(err.utf8().get_data()).
- Around line 450-452: Variant::is_null currently treats every non-OBJECT type
as null; update its logic so it only reports null for the explicit null variant
type or for an OBJECT whose internal pointer is null. Specifically, change
Variant::is_null to check the variant's null discriminator (e.g., the explicit
NULL/NIL enum value) OR (type == OBJECT && !_get_obj().obj), instead of negating
(type == OBJECT && _get_obj().obj); use the existing type enum and
_get_obj().obj accessor to implement this.
- Around line 718-729: The destructor tables store pointers to destroy<T> and
bucketFree<T> via reinterpret_cast to the generic typedefs Destructor and
BucketFree which is UB; replace those casts with type-safe adapter functions
that have the exact signatures used in the tables (e.g., a non-template void
destroy_adapter(void *p) { static_cast<T*>(p)->~T(); } and void
bucket_free_adapter(void *bucket, void *mem) { static_cast<PagedAllocator<T,
true>*>(bucket)->free(static_cast<T*>(mem)); }) and store pointers to those
adapters instead of casting destroy<T>/bucketFree<T>; update all places
referencing Destructor and BucketFree to expect the adapter signatures and
remove the reinterpret_casts involving destroy<T> and bucketFree<T>.
In `@core/variant/variant.h`:
- Around line 283-305: The needs_deinit mask omits Variant::RID causing RID
cleanup to be skipped when clear() / ~Variant() route teardown through this
mask; add (1ull << Variant::RID) back into the static constexpr std::uint64_t
needs_deinit definition so _clear_internal()'s RID teardown path is reached
during Variant destruction/clear, ensuring Variant::RID is included alongside
the other types in needs_deinit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d30bc85b-490e-41e9-a2c1-d3b083bda62c
📒 Files selected for processing (2)
core/variant/variant.cppcore/variant/variant.h
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/variant/variant.cpp (2)
2859-2874:⚠️ Potential issue | 🟡 Minor
ERR_PRINTcalled unconditionally may print empty error messages.When error codes like
CALL_ERROR_TOO_FEW_ARGUMENTS,CALL_ERROR_INSTANCE_IS_NULL, orCALL_ERROR_METHOD_NOT_CONSThit the default case,errremains empty, butERR_PRINTis still called. This could emit blank error lines depending on howERR_PRINThandles empty strings.Proposed fix to guard the print
- ERR_PRINT(err.utf8().get_data()); + if (!err.is_empty()) { + ERR_PRINT(err.utf8().get_data()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 2859 - 2874, The Variant::_variant_call_error function unconditionally calls ERR_PRINT which can emit empty lines when Callable::CallError falls into unhandled default cases; update this function (Variant::_variant_call_error) to only call ERR_PRINT when the constructed err string is non-empty (or alternatively set meaningful messages for the missing error cases such as CALL_ERROR_TOO_FEW_ARGUMENTS, CALL_ERROR_INSTANCE_IS_NULL, CALL_ERROR_METHOD_NOT_CONST), ensuring you reference Callable::CallError::CALL_ERROR_* enums and Variant::get_type_name/type/p_method when building the message so ERR_PRINT is invoked only with a non-empty, informative error string.
221-234:⚠️ Potential issue | 🟠 Major
p_type_fromis not validated before the bit shift, leading to potential undefined behavior.While
p_type_tois now validated (per prior review feedback),p_type_fromis still used directly in1ull << p_type_fromwithout validation. Ifp_type_fromis negative or ≥ 64, this is undefined behavior.Proposed fix to validate both parameters
bool Variant::can_convert(Variant::Type p_type_from, Variant::Type p_type_to) { + if ((p_type_from < NIL) || (p_type_from >= VARIANT_MAX)) { + return false; + } std::uint64_t valid_types = 0; if ((p_type_to >= NIL) && (p_type_to < VARIANT_MAX)) { valid_types = TYPE_CAST_TABLE[p_type_to]; } return (valid_types & (1ull << p_type_from)) != 0; } bool Variant::can_convert_strict(Variant::Type p_type_from, Variant::Type p_type_to) { + if ((p_type_from < NIL) || (p_type_from >= VARIANT_MAX)) { + return false; + } std::uint64_t valid_types = 0; if ((p_type_to >= NIL) && (p_type_to < VARIANT_MAX)) { valid_types = TYPE_STRICT_CAST_TABLE[p_type_to]; } return (valid_types & (1ull << p_type_from)) != 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 221 - 234, Both Variant::can_convert and Variant::can_convert_strict validate p_type_to but use p_type_from unchecked in the bit-shift (1ull << p_type_from), which can cause UB for negative or >=64 values; update both functions to validate p_type_from (ensure p_type_from >= NIL and p_type_from < VARIANT_MAX and also < 64) before performing the shift, only compute the mask when both indices are valid, and keep using TYPE_CAST_TABLE and TYPE_STRICT_CAST_TABLE for the lookup when p_type_to is valid.
🧹 Nitpick comments (2)
core/variant/variant.cpp (2)
2810-2829: Unreachable code at line 2829.Line 2829 (
return p_lhs < p_rhs;) is unreachable because all code paths through the two switches either return explicitly or hit adefaultcase that returns.Proposed fix to remove dead code
switch (p_rhs.get_type()) { case Variant::STRING: return StringName::AlphCompare::compare(lhs, *VariantInternal::get_string(&p_rhs)); case Variant::STRING_NAME: return StringName::AlphCompare::compare(lhs, *VariantInternal::get_string_name(&p_rhs)); default: return p_lhs < p_rhs; } - return p_lhs < p_rhs; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 2810 - 2829, The final unconditional "return p_lhs < p_rhs;" is unreachable because both switch blocks on p_lhs.get_type() and p_rhs.get_type() always return (including their default cases); remove that dead return at the end of the function (after the second switch) so only the meaningful returns remain; locate the code around variables/identifiers lhs, p_lhs, p_rhs, Variant::STRING, Variant::STRING_NAME, VariantInternal::get_string/get_string_name and StringName::AlphCompare::compare to make this change.
90-144: Typo in comment: "callbale" should be "callable".Line 120 and 176 have "callbale" instead of "callable".
Also, consider using
1ull <<consistently throughout the table (even for types < 32) to avoid potential issues if enum values change in the future.Proposed fix for typos
- (1 << Variant::CALLABLE), // callbale + (1 << Variant::CALLABLE), // callable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 90 - 144, Fix the typo and make bit-shifts consistent: update the comment labeling the entry for Variant::CALLABLE in TYPE_CAST_TABLE from "callbale" to "callable" and change any plain (1 << Variant::...) shifts in TYPE_CAST_TABLE to use 1ull << Variant::... (e.g., replace (1 << Variant::BOOL) with (1ull << Variant::BOOL)) so all shifts are performed with unsigned 64-bit literals; verify entries referencing Variant::CALLABLE, Variant::SIGNAL, Variant::DICTIONARY, etc., are updated accordingly.
🤖 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.cpp`:
- Around line 2859-2874: The Variant::_variant_call_error function
unconditionally calls ERR_PRINT which can emit empty lines when
Callable::CallError falls into unhandled default cases; update this function
(Variant::_variant_call_error) to only call ERR_PRINT when the constructed err
string is non-empty (or alternatively set meaningful messages for the missing
error cases such as CALL_ERROR_TOO_FEW_ARGUMENTS, CALL_ERROR_INSTANCE_IS_NULL,
CALL_ERROR_METHOD_NOT_CONST), ensuring you reference
Callable::CallError::CALL_ERROR_* enums and Variant::get_type_name/type/p_method
when building the message so ERR_PRINT is invoked only with a non-empty,
informative error string.
- Around line 221-234: Both Variant::can_convert and Variant::can_convert_strict
validate p_type_to but use p_type_from unchecked in the bit-shift (1ull <<
p_type_from), which can cause UB for negative or >=64 values; update both
functions to validate p_type_from (ensure p_type_from >= NIL and p_type_from <
VARIANT_MAX and also < 64) before performing the shift, only compute the mask
when both indices are valid, and keep using TYPE_CAST_TABLE and
TYPE_STRICT_CAST_TABLE for the lookup when p_type_to is valid.
---
Nitpick comments:
In `@core/variant/variant.cpp`:
- Around line 2810-2829: The final unconditional "return p_lhs < p_rhs;" is
unreachable because both switch blocks on p_lhs.get_type() and p_rhs.get_type()
always return (including their default cases); remove that dead return at the
end of the function (after the second switch) so only the meaningful returns
remain; locate the code around variables/identifiers lhs, p_lhs, p_rhs,
Variant::STRING, Variant::STRING_NAME,
VariantInternal::get_string/get_string_name and StringName::AlphCompare::compare
to make this change.
- Around line 90-144: Fix the typo and make bit-shifts consistent: update the
comment labeling the entry for Variant::CALLABLE in TYPE_CAST_TABLE from
"callbale" to "callable" and change any plain (1 << Variant::...) shifts in
TYPE_CAST_TABLE to use 1ull << Variant::... (e.g., replace (1 << Variant::BOOL)
with (1ull << Variant::BOOL)) so all shifts are performed with unsigned 64-bit
literals; verify entries referencing Variant::CALLABLE, Variant::SIGNAL,
Variant::DICTIONARY, etc., are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c273a6c7-a60a-4e66-8912-ebd20728852f
📒 Files selected for processing (1)
core/variant/variant.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.cpp`:
- Line 176: Fix the typo in the inline comment next to the Variant::CALLABLE
flag: change "callbale" to "callable" so the comment reads correctly; locate the
bitmask entry "(1 << Variant::CALLABLE), // callbale" in variant.cpp and update
the trailing comment to "// callable".
- Around line 221-235: The functions Variant::can_convert and
Variant::can_convert_strict validate p_type_to but still perform undefined
shifts with p_type_from; add a range check for p_type_from before doing (1ull <<
p_type_from). Specifically, ensure p_type_from is within the valid enum range
(e.g., p_type_from >= NIL && p_type_from < VARIANT_MAX) and also less than 64
(or use VARIANT_MAX <= 64 assumption) and only then perform the shift and mask
against TYPE_CAST_TABLE / TYPE_STRICT_CAST_TABLE; if the check fails return
false. Apply the same guard in both can_convert and can_convert_strict and use a
uint64_t temporary for the shifted bit after validation.
- Around line 2858-2876: The switch in Variant::_variant_call_error falls
through due to missing break statements and returns in default so ERR_PRINT
never runs; update each case in Variant::_variant_call_error to assign the
correct message and terminate the case with a break, fix the
CALL_ERROR_TOO_FEW_ARGUMENTS message to "Too few arguments for method
'p_method'" (or similar), and change the default so it does not return
immediately but sets a fallback error string (e.g. "Unknown call error") before
the final ERR_PRINT call; ensure you reference Callable::CallError enum values
(CALL_ERROR_INVALID_ARGUMENT, CALL_ERROR_INVALID_METHOD,
CALL_ERROR_TOO_MANY_ARGUMENTS, CALL_ERROR_TOO_FEW_ARGUMENTS,
CALL_ERROR_INSTANCE_IS_NULL, CALL_ERROR_METHOD_NOT_CONST) and the function
Variant::get_type_name/type as needed when composing messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90dd6f71-a7b0-4d23-88e3-f4011898a908
📒 Files selected for processing (1)
core/variant/variant.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
core/variant/variant.cpp (4)
221-227:⚠️ Potential issue | 🟠 MajorValidate
p_type_frombefore shifting.Only
p_type_tois range-checked. Lines 226 and 234 still do1ull << p_type_fromon an unchecked enum, so negative or out-of-range values hit undefined behavior.Suggested fix
bool Variant::can_convert(Variant::Type p_type_from, Variant::Type p_type_to) { - std::uint64_t valid_types = 0; - if ((p_type_to >= NIL) && (p_type_to < VARIANT_MAX)) { - valid_types = TYPE_CAST_TABLE[p_type_to]; - } - return (valid_types & (1ull << p_type_from)) != 0; + if ((p_type_from < NIL) || (p_type_from >= VARIANT_MAX) || + (p_type_to < NIL) || (p_type_to >= VARIANT_MAX)) { + return false; + } + return (TYPE_CAST_TABLE[p_type_to] & (1ull << p_type_from)) != 0; } bool Variant::can_convert_strict(Variant::Type p_type_from, Variant::Type p_type_to) { - std::uint64_t valid_types = 0; - if ((p_type_to >= NIL) && (p_type_to < VARIANT_MAX)) { - valid_types = TYPE_STRICT_CAST_TABLE[p_type_to]; - } - return (valid_types & (1ull << p_type_from)) != 0; + if ((p_type_from < NIL) || (p_type_from >= VARIANT_MAX) || + (p_type_to < NIL) || (p_type_to >= VARIANT_MAX)) { + return false; + } + return (TYPE_STRICT_CAST_TABLE[p_type_to] & (1ull << p_type_from)) != 0; }Also applies to: 229-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 221 - 227, The code in Variant::can_convert uses (1ull << p_type_from) without validating p_type_from, causing UB for negative or out-of-range enum values; update Variant::can_convert to range-check p_type_from (e.g., ensure p_type_from >= NIL && p_type_from < VARIANT_MAX) before performing the left shift and return false if out of range; apply the same validation to the duplicate block referenced around lines 229-235 that also shifts by p_type_from and ensure you use TYPE_CAST_TABLE[p_type_to] only after both p_type_to and p_type_from are validated.
2867-2870:⚠️ Potential issue | 🟡 MinorUse the correct message for
CALL_ERROR_TOO_FEW_ARGUMENTS.Line 2868 still falls into the
"Too many arguments"text, so the logged diagnosis is inverted for that error.Suggested fix
case Callable::CallError::CALL_ERROR_TOO_MANY_ARGUMENTS: - case Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS: err = "Too many arguments for method '" + p_method + "'"; break; + case Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS: + err = "Too few arguments for method '" + p_method + "'"; + break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 2867 - 2870, The error string for Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS is incorrect—both cases set err to "Too many arguments for method '" + p_method + "'". Update the branch handling CALL_ERROR_TOO_FEW_ARGUMENTS (the switch case for Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS) to set err to "Too few arguments for method '" + p_method + "'" (or otherwise produce the correct "Too few" message) so err reflects the actual error; keep the CALL_ERROR_TOO_MANY_ARGUMENTS case returning the "Too many arguments..." message and ensure p_method is used in both.
2835-2840:⚠️ Potential issue | 🟠 MajorPacked arrays still need to be marked shared.
Packed arrays are reference-counted and assignments in this file keep shared backing storage, so
is_type_shared()/is_shared()now incorrectly reportfalsefor everyPACKED_*_ARRAY.Suggested fix
static const std::uint64_t IS_SHARED = (1ull << Variant::OBJECT) | (1ull << Variant::ARRAY) | - (1ull << Variant::DICTIONARY); + (1ull << Variant::DICTIONARY) | + (1ull << Variant::PACKED_BYTE_ARRAY) | + (1ull << Variant::PACKED_INT32_ARRAY) | + (1ull << Variant::PACKED_INT64_ARRAY) | + (1ull << Variant::PACKED_FLOAT32_ARRAY) | + (1ull << Variant::PACKED_FLOAT64_ARRAY) | + (1ull << Variant::PACKED_STRING_ARRAY) | + (1ull << Variant::PACKED_VECTOR2_ARRAY) | + (1ull << Variant::PACKED_VECTOR3_ARRAY) | + (1ull << Variant::PACKED_COLOR_ARRAY) | + (1ull << Variant::PACKED_VECTOR4_ARRAY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 2835 - 2840, IS_SHARED mask omits packed array types so Variant::is_type_shared (and is_shared()) returns false for PACKED_*_ARRAYs; update the IS_SHARED bitmask to include all PACKED_*_ARRAY Variant::Type values (e.g., PACKED_INT_ARRAY, PACKED_REAL_ARRAY, PACKED_VECTOR2_ARRAY, PACKED_VECTOR3_ARRAY, PACKED_COLOR_ARRAY, PACKED_STRING_ARRAY, PACKED_NODE_ARRAY, PACKED_RAW_ARRAY or whatever PACKED_*_ARRAY enum names exist) by OR-ing (1ull << Variant::PACKED_... ) for each packed array type inside the IS_SHARED initializer so packed arrays are treated as shared. Ensure you reference the Variant::Type enum names exactly as declared when adding them.
718-729:⚠️ Potential issue | 🔴 CriticalAvoid calling cleanup helpers through casted function pointers.
The
destroy<T>/bucketFree<T>functions are cast to incompatible erased function-pointer types and then invoked through those casted types. That's undefined behavior in C++, so_clear_internal()becomes ABI-dependent.Suggested fix
template <typename T> -void destroy(T *s) { - reinterpret_cast<T *>(s)->~T(); +static void destroy_adapter(void *p_mem) { + static_cast<T *>(p_mem)->~T(); } template <typename T> -void bucketFree(PagedAllocator<T, true> *bucket, void *mem) { - ((PagedAllocator<T, true> *)bucket)->free((T *)mem); +static void bucket_free_adapter(void *p_bucket, void *p_mem) { + static_cast<PagedAllocator<T, true> *>(p_bucket)->free(static_cast<T *>(p_mem)); } @@ - (Destructor)destroy<String>, + destroy_adapter<String>, @@ - (Destructor)destroy<Transform2D>, + destroy_adapter<Transform2D>, @@ - (BucketFree)bucketFree<Variant::Pools::BucketSmall>, + bucket_free_adapter<Variant::Pools::BucketSmall>, @@ - (BucketFree)bucketFree<Variant::Pools::BucketMedium>, + bucket_free_adapter<Variant::Pools::BucketMedium>, @@ - (BucketFree)bucketFree<Variant::Pools::BucketLarge>, + bucket_free_adapter<Variant::Pools::BucketLarge>,Also applies to: 732-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/variant/variant.cpp` around lines 718 - 729, The code currently casts template functions destroy<T> and bucketFree<T> to the erased types Destructor and BucketFree which is UB; instead, implement non-templated trampoline functions with the exact erased signatures (e.g. destroy_trampoline(void* p) and bucketFree_trampoline(void* bucket, void* mem)) that inside cast to T* and call the destructor or bucket->free, and assign those trampolines to Destructor and BucketFree; update wherever destroy<T>/bucketFree<T> were cast (notably where Destructor/BucketFree are stored/used and in _clear_internal) to use these trampolines so no incompatible function-pointer casts occur.
🤖 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.cpp`:
- Around line 209-218: The static HashMap<string, Variant::Type> type_names in
Variant::get_type_by_name is lazily populated with mutation on first use (via
is_empty()), creating a data race; change to build and populate type_names in a
thread-safe static initializer (e.g. initialize the static via a function-local
static constructed from a lambda or helper that fills the map by iterating
get_type_name((Type)i) for i from NIL to VARIANT_MAX) so no concurrent writes
occur in Variant::get_type_by_name; keep the lookup logic using
type_names.getptr(p_type_name) and return VARIANT_MAX when not found.
---
Duplicate comments:
In `@core/variant/variant.cpp`:
- Around line 221-227: The code in Variant::can_convert uses (1ull <<
p_type_from) without validating p_type_from, causing UB for negative or
out-of-range enum values; update Variant::can_convert to range-check p_type_from
(e.g., ensure p_type_from >= NIL && p_type_from < VARIANT_MAX) before performing
the left shift and return false if out of range; apply the same validation to
the duplicate block referenced around lines 229-235 that also shifts by
p_type_from and ensure you use TYPE_CAST_TABLE[p_type_to] only after both
p_type_to and p_type_from are validated.
- Around line 2867-2870: The error string for
Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS is incorrect—both cases set
err to "Too many arguments for method '" + p_method + "'". Update the branch
handling CALL_ERROR_TOO_FEW_ARGUMENTS (the switch case for
Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS) to set err to "Too few
arguments for method '" + p_method + "'" (or otherwise produce the correct "Too
few" message) so err reflects the actual error; keep the
CALL_ERROR_TOO_MANY_ARGUMENTS case returning the "Too many arguments..." message
and ensure p_method is used in both.
- Around line 2835-2840: IS_SHARED mask omits packed array types so
Variant::is_type_shared (and is_shared()) returns false for PACKED_*_ARRAYs;
update the IS_SHARED bitmask to include all PACKED_*_ARRAY Variant::Type values
(e.g., PACKED_INT_ARRAY, PACKED_REAL_ARRAY, PACKED_VECTOR2_ARRAY,
PACKED_VECTOR3_ARRAY, PACKED_COLOR_ARRAY, PACKED_STRING_ARRAY,
PACKED_NODE_ARRAY, PACKED_RAW_ARRAY or whatever PACKED_*_ARRAY enum names exist)
by OR-ing (1ull << Variant::PACKED_... ) for each packed array type inside the
IS_SHARED initializer so packed arrays are treated as shared. Ensure you
reference the Variant::Type enum names exactly as declared when adding them.
- Around line 718-729: The code currently casts template functions destroy<T>
and bucketFree<T> to the erased types Destructor and BucketFree which is UB;
instead, implement non-templated trampoline functions with the exact erased
signatures (e.g. destroy_trampoline(void* p) and bucketFree_trampoline(void*
bucket, void* mem)) that inside cast to T* and call the destructor or
bucket->free, and assign those trampolines to Destructor and BucketFree; update
wherever destroy<T>/bucketFree<T> were cast (notably where Destructor/BucketFree
are stored/used and in _clear_internal) to use these trampolines so no
incompatible function-pointer casts occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c612872-cf9b-42a3-b90c-f89b0cc7390b
📒 Files selected for processing (1)
core/variant/variant.cpp
JoltedJon
reviewed
Mar 13, 2026
JoltedJon
previously approved these changes
Mar 13, 2026
JoltedJon
previously approved these changes
Mar 13, 2026
JoltedJon
approved these changes
Mar 14, 2026
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
4 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.
Problem
I got nerd sniped by Tekk into making significant reductions to this file
What this PR changes
Significantly cleans up the variant implementation such that it is a lot more maintainable and anything with clear cases that reasonably could be compressed was made into clear cases that were reasonably compressed
Result
A much cleaner and smaller variant implementation file
Testing
All existing tests pass. No new tests were added nor were there old tests that were removed.
Summary by CodeRabbit