Skip to content

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

Merged
dsnopek merged 1 commit into
godotengine:masterfrom
dsnopek:fix-null-object-arguments
Apr 29, 2024
Merged

Correctly handle Object * arguments that were encoded as nullptr#1405
dsnopek merged 1 commit into
godotengine:masterfrom
dsnopek:fix-null-object-arguments

Conversation

@dsnopek

@dsnopek dsnopek commented Mar 5, 2024

Copy link
Copy Markdown
Collaborator

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

@AThousandShips AThousandShips left a comment

Copy link
Copy Markdown
Member

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

Comment thread include/godot_cpp/core/method_ptrcall.hpp Outdated
Comment thread include/godot_cpp/core/method_ptrcall.hpp Outdated
@dsnopek
dsnopek force-pushed the fix-null-object-arguments branch from 6d1b443 to db21a6e Compare April 8, 2024 16:06
@dsnopek

dsnopek commented Apr 8, 2024

Copy link
Copy Markdown
Collaborator Author

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

reduz commented Apr 29, 2024

Copy link
Copy Markdown
Member

Looks great!

@dsnopek

dsnopek commented Apr 29, 2024

Copy link
Copy Markdown
Collaborator Author

Thanks for the review!

@dsnopek
dsnopek merged commit 2cd3d39 into godotengine:master Apr 29, 2024
@dsnopek

dsnopek commented May 17, 2024

Copy link
Copy Markdown
Collaborator Author

Cherry-picked for 4.2 in PR #1465

@dsnopek

dsnopek commented May 17, 2024

Copy link
Copy Markdown
Collaborator Author

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