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

Use different Object/Ref encoding for virtual methods #1135

Closed

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Jun 6, 2023

This attempts to implement option nr 3 from issue #1119

Basically, it just decodes objects from Object ** in virtual methods, and from Object * in all other situations.

There shouldn't affect performance - basically PtrToArg<>::convert() expands to different code when used for a virtual vs non-virtual method.

(It's an alternative to #1123 which is implementing option nr 2)

@dsnopek dsnopek added bug This has been identified as a bug regression labels Jun 6, 2023
@dsnopek dsnopek added this to the 4.1 milestone Jun 6, 2023
@dsnopek dsnopek requested a review from a team as a code owner June 6, 2023 17:03
@dsnopek dsnopek force-pushed the object-pointer-and-pointer-pointer branch from 991f044 to 1d658b6 Compare June 6, 2023 17:10
@dsnopek dsnopek force-pushed the object-pointer-and-pointer-pointer branch from 1d658b6 to adfc303 Compare June 6, 2023 18:22
@saki7
Copy link
Contributor

saki7 commented Jun 7, 2023

Thanks for illustrating this solution.

IMHO this approach (option nr 3) should be avoided at all costs because it seems to require a lot of ad-hoc differentiations. Even though we have those helpful if constexpr in our language, it would require same amount of effort on all other bindings. That would become a maintenance burden in future.

Quoting from #1119 (comment):

Given that we're about 1 week passed the feature freeze and Godot PR godotengine/godot#77410 hasn't gotten review from any of the necessary stakeholders, this may end up being the way we need to go for Godot 4.1.

Few hours ago godotengine/godot#77410 has received an approval. We should choose that one alongside with #1123.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Jun 7, 2023

Superceded by PR #1123

@dsnopek dsnopek closed this Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived bug This has been identified as a bug regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants