Skip to content
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.

Sign up for GitHub

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

Jump to bottom

C#: Fix errors when creating Variant from null array #89756

Merged
1 commit merged into from
Mar 24, 2024
+18 −3
Merged

C#: Fix errors when creating Variant from null array #89756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/VariantUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,13 +235,28 @@ public static godot_variant CreateFromPackedColorArray(Span<Color> from)
}

public static godot_variant CreateFromSystemArrayOfStringName(Span<StringName> from)
=> CreateFromArray(new Collections.Array(from));
{
if (from == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird to me, Spans are value types so they can't be null. The only reason why they can be compared to null is because there is an implicit conversion from arrays to spans and the null literal can be converted to array so basically this is doing this:

if (from == (Span<StringName>)((StringName[])null))
    return default;

Which means this also returns a default Variant for empty arrays and I'm not sure we want that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok because for empty arrays, the span won't be null (as its _pointer points to that array). So, it returns default Variant only when the array is null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, I was thinking whether we should allow creating Godot.Array from null but on second thought I think the changes in this PR are more correct.

return default;
using var fromGodot = new Collections.Array(from);
return CreateFromArray((godot_array)fromGodot.NativeValue);
}

public static godot_variant CreateFromSystemArrayOfNodePath(Span<NodePath> from)
=> CreateFromArray(new Collections.Array(from));
{
if (from == null)
return default;
using var fromGodot = new Collections.Array(from);
return CreateFromArray((godot_array)fromGodot.NativeValue);
}

public static godot_variant CreateFromSystemArrayOfRid(Span<Rid> from)
=> CreateFromArray(new Collections.Array(from));
{
if (from == null)
return default;
using var fromGodot = new Collections.Array(from);
return CreateFromArray((godot_array)fromGodot.NativeValue);
}

public static godot_variant CreateFromSystemArrayOfGodotObject(GodotObject[]? from)
{
Expand Down
Loading