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

GDNative: fix StringName equal and less operators #63104

Merged
merged 1 commit into from
Jul 17, 2022

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Jul 17, 2022

The new function accepts a pointer, where it constructs a StringName in-place:

void GDAPI godot_string_name_new(godot_string_name *r_dest, const godot_string *p_name) {
	StringName *dest = (StringName *)r_dest;
	const String *name = (const String *)p_name;
	memnew_placement(dest, StringName(*name));
}

This means that it's possible to have different pointers to the same StringName.

However, the equal and less methods only compare that pointer:

godot_bool GDAPI godot_string_name_operator_equal(const godot_string_name *p_self, const godot_string_name *p_other) {
	const StringName *self = (const StringName *)p_self;
	const StringName *other = (const StringName *)p_other;
	return self == other;
}

godot_bool GDAPI godot_string_name_operator_less(const godot_string_name *p_self, const godot_string_name *p_other) {
	const StringName *self = (const StringName *)p_self;
	const StringName *other = (const StringName *)p_other;
	return self < other;
}

And to do that, they wouldn't need to first cast to StringName*. It looks like forgotten dereferencing.

@Bromeon
Copy link
Contributor Author

Bromeon commented Jul 17, 2022

I haven't had the chance to test this change in godot-rust yet, hopefully will get around to it later today.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I'll wait for feedback on how it fares in godot-rust to merge.

@Bromeon
Copy link
Contributor Author

Bromeon commented Jul 17, 2022

Added tests in godot-rust/gdnative#912, they are currently failing as expected.

When I compile Godot locally with the change in this PR, they pass. I couldn't use this exact revision since godot-rust is still based on Godot 3.4, so I cherry-picked my commit from here onto an older Godot revision (for my local tests).

TLDR: ready from my side.

@akien-mga akien-mga merged commit 06a12c0 into godotengine:3.x Jul 17, 2022
@akien-mga
Copy link
Member

Thanks!

@Bromeon Bromeon deleted the 3.x branch July 17, 2022 15:19
bors bot added a commit to godot-rust/gdnative that referenced this pull request Jul 17, 2022
912: Test + workaround: `Eq/Ord` for `StringName` r=Bromeon a=Bromeon

Upstream bug, see godotengine/godot#63104

This adds tests for `StringName`'s equality and ordering relation.

Co-authored-by: Jan Haller <bromeon@gmail.com>
KarimHamidou added a commit to KarimHamidou/godot-rust that referenced this pull request Feb 6, 2023
912: Test + workaround: `Eq/Ord` for `StringName` r=Bromeon a=Bromeon

Upstream bug, see godotengine/godot#63104

This adds tests for `StringName`'s equality and ordering relation.

Co-authored-by: Jan Haller <bromeon@gmail.com>
GuilhermeOrceziae added a commit to GuilhermeOrceziae/godot-rust that referenced this pull request Feb 9, 2023
912: Test + workaround: `Eq/Ord` for `StringName` r=Bromeon a=Bromeon

Upstream bug, see godotengine/godot#63104

This adds tests for `StringName`'s equality and ordering relation.

Co-authored-by: Jan Haller <bromeon@gmail.com>
hesuteia added a commit to hesuteia/godot-rust that referenced this pull request Feb 11, 2023
912: Test + workaround: `Eq/Ord` for `StringName` r=Bromeon a=Bromeon

Upstream bug, see godotengine/godot#63104

This adds tests for `StringName`'s equality and ordering relation.

Co-authored-by: Jan Haller <bromeon@gmail.com>
ecobiubiu added a commit to ecobiubiu/open-rust that referenced this pull request Mar 30, 2023
912: Test + workaround: `Eq/Ord` for `StringName` r=Bromeon a=Bromeon

Upstream bug, see godotengine/godot#63104

This adds tests for `StringName`'s equality and ordering relation.

Co-authored-by: Jan Haller <bromeon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants