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

Correctly handle Object * arguments that were encoded as nullptr #1405

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Mar 5, 2024

Fixes godotengine/godot#86478
Fixes #1056

This is an alternative to PR godotengine/godot#87613, fixing the issue in godot-cpp rather than in Godot itself. I explained my reasons for wanting to fix it in godot-cpp in this comment.

This uses a ternary expression to match the code in the encode() method just below the convert() method this PR changes:

	_FORCE_INLINE_ static void encode(T *p_var, void *p_ptr) {
		*reinterpret_cast<const void **>(p_ptr) = p_var ? p_var->_owner : nullptr;
	}

I think having the symmetry between the encode/decode methods is nice, even though it leads to a very long line of code. :-)

This PR also adds an automated test which will crash on master, but succeed with this PR.

@dsnopek dsnopek added bug This has been identified as a bug crash cherrypick:4.1 cherrypick:4.2 labels Mar 5, 2024
@dsnopek dsnopek added this to the 4.x milestone Mar 5, 2024
@dsnopek dsnopek requested a review from a team as a code owner March 5, 2024 17:25
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This makes sense and is elegant, matches below as stated

include/godot_cpp/core/method_ptrcall.hpp Outdated Show resolved Hide resolved
include/godot_cpp/core/method_ptrcall.hpp Outdated Show resolved Hide resolved
@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 8, 2024

I've updated the PR per @reduz's requests, as well as made the encode() methods consist with that change.

And I just posted PR godotengine/godot#90394 to make the same change in the equivalent code in Godot.

@dsnopek dsnopek requested a review from reduz April 8, 2024 16:17
@reduz
Copy link
Member

reduz commented Apr 29, 2024

Looks great!

@dsnopek
Copy link
Collaborator Author

dsnopek commented Apr 29, 2024

Thanks for the review!

@dsnopek dsnopek merged commit 2cd3d39 into godotengine:master Apr 29, 2024
12 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented May 17, 2024

Cherry-picked for 4.2 in PR #1465

@dsnopek
Copy link
Collaborator Author

dsnopek commented May 17, 2024

Cherry-picked for 4.1 in PR #1466

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug crash
Projects
None yet
4 participants