-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core] Fix sharing of typed arrays from constructor #89197
Conversation
d712990
to
52d47c4
Compare
@@ -46,10 +46,15 @@ class TypedArray : public Array { | |||
_ref(p_array); | |||
} | |||
_FORCE_INLINE_ TypedArray(const Variant &p_variant) : | |||
Array(Array(p_variant), Variant::OBJECT, T::get_class_static(), Variant()) { | |||
TypedArray(Array(p_variant)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified this by delegating to avoid copying code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all in to simplify the code.
_FORCE_INLINE_ TypedArray(const Array &p_array) { | ||
set_typed(Variant::OBJECT, T::get_class_static(), Variant()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_FORCE_INLINE_ TypedArray(const Array &p_array) { | |
set_typed(Variant::OBJECT, T::get_class_static(), Variant()); | |
_FORCE_INLINE_ TypedArray(const Array &p_array) : | |
TypedArray() { |
Alternatively, to reduce duplication even further
_ref(p_array); | ||
} else { | ||
assign(p_array); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively this block can be replaced by:
if (p_array.get_typed_builtin() == Variant::OBJECT && p_array.get_typed_class_name() == T::get_class_static() && p_array.get_typed_script() == Variant()) {
_ref(p_array);
} else {
set_typed(Variant::OBJECT, T::get_class_static(), Variant());
assign(p_array);
}
To avoid doing the typing part, though in either case it will allocate private data possibly unnecessarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though in either case it will allocate private data possibly unnecessarily
Then, I would keep the simplest version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I will and a future cleanup or improvement can hand that
52d47c4
to
b3cb1c8
Compare
b3cb1c8
to
09460d3
Compare
I just tried this PR and it fixes the issue using the minimum reproduction code that I shared in #89191. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
After this change, is the Array(const Array &p_base, uint32_t p_type, const StringName &p_class_name, const Variant &p_script)
constructor even used anywhere? If not, I wonder if it should be removed? Anyway, not important questions for now, things could be cleaned up more later :-)
I think it's used in scripting, as the template can't be used in dynamic code, only static code, so in GDScript etc. the explicit constructor is relevant Edit: it is used, for example here: godot/modules/gdscript/gdscript_vm.cpp Line 1672 in 93fc9b8
|
Thanks! |
Thank you! |
This matches the copy constructor of
Array
when same typed, which I'd say is expected behavior