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.

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

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

Merged
1 commit merged into from
Mar 24, 2024

Conversation

zaevi
Copy link
Contributor

@zaevi zaevi commented Mar 21, 2024

@zaevi zaevi requested a review from a team as a code owner March 21, 2024 16:05
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 21, 2024
Copy link
Member

@paulloz paulloz left a comment

Choose a reason for hiding this comment

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

Good catch! I think this is fine, yep.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, I'd like to understand the PR a bit better before approving.

Returns default Variant (nil) if the Span<*> is null.

I'm not sure what you mean by this, Spans can't be null because they are value types. Can you explain why this change is needed? Do empty arrays work before/after this PR?

The current issue is probably because of checks like:

public Array(Span<StringName> array)
{
if (array == null)
throw new ArgumentNullException(nameof(array));

I imagine this was a copy-paste mistake. But then Array.Empty<StringName>() should throw as well.

@@ -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.

@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
akien-mga added a commit that referenced this pull request Mar 24, 2024
C#: Fix errors when creating `Variant` from null array
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants