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

Fix comment in gdextension_interface.h #83415

Merged

Conversation

touilleMan
Copy link
Member

GDExtensionInterfaceGetGodotVersion *get_godot_version = (GDExtensionInterfaceGetGodotVersion)p_get_proc_address("get_godot_version");

The comment incorrectly defines a pointer type (note the cast on the right side of the equal is correct !)

This is misleading for the unsuspecting end-user, which is tempted to "solve" this by doing a de-reference... hence leading to a segfault 😞

@touilleMan touilleMan requested a review from a team as a code owner October 15, 2023 22:46
@touilleMan
Copy link
Member Author

On top of that, I wonder if it wouldn't be better to also add an intermediary void* cast:

GDExtensionInterfaceGetGodotVersion get_godot_version = (GDExtensionInterfaceGetGodotVersion)(void*)p_get_proc_address("get_godot_version");

This is useful for some functions to avoid -Wcast-function-type errors, for instance when doing p_get_proc_address("variant_get_ptr_constructor"):

warning: cast between incompatible function types from ‘GDExtensionInterfaceFunctionPtr’ {aka ‘void (*)()’} to ‘void (* (*)(GDExtensionVariantType,  int32_t))(void *, const void * const*)’ {aka ‘void (* (*)(GDExtensionVariantType,  int))(void *, const void * const*)’} [-Wcast-function-type]

what do you think ?

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me

(Sorry, this was probably my mistake :-))

@dsnopek
Copy link
Contributor

dsnopek commented Oct 15, 2023

On top of that, I wonder if it wouldn't be better to also add an intermediary void* cast

Hm, I guess so. However, I don't want to end up getting too deep into details, so it doesn't distract from what we're trying to show.

@touilleMan touilleMan force-pushed the fix-comment-gdextension_interface branch from 1322f1e to cbcdf38 Compare October 15, 2023 23:12
@touilleMan
Copy link
Member Author

@dsnopek I've updated the PR to add the void* cast

I think it's better to have the user wonder why the void* is for then remove it and see the error, than to have him scratch his head on how to avoid this -Wcast-function-type (especially given this error has different form depending on gcc/clang/msvc)

@Bromeon
Copy link
Contributor

Bromeon commented Oct 16, 2023

I think it's better to have the user wonder why the void* is for then remove it and see the error, than to have him scratch his head on how to avoid this -Wcast-function-type (especially given this error has different form depending on gcc/clang/msvc)

I don't know, you trade one obscurity for another. Now the user wonders why there is an intermediate void* cast.

This is also rather compiler-specific; furthermore C++ code would likely use reinterpret_cast. I would probably omit the void* cast but add a a paragraph that mentions this workaround in case of warnings. That way, it's at least explained 🙂

@touilleMan
Copy link
Member Author

@Bromeon I've udpated the PR to replace the cast in the snippet by a small explanation as you suggested

@akien-mga akien-mga changed the title Fix comment in gdextension_interface.h Fix comment in gdextension_interface.h Oct 16, 2023
@touilleMan touilleMan force-pushed the fix-comment-gdextension_interface branch from 312fc1b to 5aa9f1c Compare October 16, 2023 19:41
@touilleMan touilleMan merged commit fd33c7b into godotengine:master Oct 16, 2023
4 checks passed
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.

4 participants