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 memdelete() of Object using pointer to secondary parent class #88688

Closed

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 22, 2024

Fixes #88613

There is a pattern used in a very small number of places in Godot, in order to simulate "interfaces" (which C++ doesn't directly support).

Here's how it works:

  • Declare an abstract class (aka a class with all pure virtual methods) to act as the interface
  • In classes that want to implement that interface, use multiple inheritance to extend Object (or some other ancestor of Object) as the first parent class, and then extend the abstract class as a secondary parent class
  • Then methods that want to accept instances that implement this "interface" can specify the secondary parent class as the type

This is used by a couple of classes in the OpenXR module:

  • OpenXRExtensionWrapper
  • OpenXRGraphicsExtensionWrapper (which extends the previous one)
  • OpenXRCompositionLayerProvider

This all works fine, except when you keep a reference to one of these types and then later try to memdelete() it, which is done in the case of OpenXRExtensionWrapper and is the source of the bug from issue #88613

This PR allows developers to mark one of these interface classes using a new IS_POSSIBLE_GDCLASS() macro, that will tell memdelete() to do some additional work that will clean everything up correctly.

Given how uncommon this pattern is in Godot, I could understand not wanting to accept this general change to memdelete() and instead have the OpenXR implement its own bespoke solution. However, I thought I would propose it and see what folks thought :-)

@akien-mga
Copy link
Member

Given that the class still needs to opt in to this new core behavior with IS_POSSIBLE_GDCLASS, I feel the incentive to get this in core isn't that clear cut. The local solution in #88689 seems much simpler, and both cases still require recognizing and properly handling the uncommon pattern used by such classes.

If we could make it automagical so this pattern works out of the box, the core approach would make a lot of sense IMO (provided it doesn't add too much complexity or impact negatively the other, more common patterns).

But that's just my non-expert opinion. I'd wait to get input from @reduz and @RandomShaper who might like this approach. That might take a while to assess, so in the short temr I suggest we merge #88689 (and this PR can then be rebased to undo the change for #88689 that wouldn't be needed if this approach is merged).

@RandomShaper
Copy link
Member

Without having assessed personally the classes involved in the issue, this is my take:

  • First, I agree that this PR (although I acknowledge its advanced C++ merit!) is too much to add to something as core as Object for such a marginal use case.
  • Second, I'm sure an alternative design is possible where multiple inheritance is not needed. Again, I haven't checked the issue in detail, but this sounds like some of those cases where composition can be used once simple inheritance doesn't scale.

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 23, 2024

If we could make it automagical so this pattern works out of the box, the core approach would make a lot of sense IMO (provided it doesn't add too much complexity or impact negatively the other, more common patterns).

It's possible to write the template to automagically detect the situation, but, unfortunately, it requires the full definition of Object be available, which we can't really do for all the places that memory.h is being used.

  • First, I agree that this PR (although I acknowledge its advanced C++ merit!) is too much to add to something as core as Object for such a marginal use case.

Understood! I guess I'll just close this one then :-)

@dsnopek dsnopek closed this Feb 23, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Feb 23, 2024
@dsnopek dsnopek deleted the memdelete-multiple-inheritence branch July 22, 2024 15:31
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.

Crash on exit when using OpenXRExtensionWrapperExtension
4 participants