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

Slightly improved support for GPtrArray #839

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

hol430
Copy link
Contributor

@hol430 hol430 commented Mar 30, 2023

  • I agree that my contribution may be licensed either under MIT or any version of LGPL license.

This is needed for the gtksourceview bindings (specifically, the completion API, which gets only partially rendered without this, which results in build errors).

I'm not totally certain of the internal -> public conversion so let me know if that looks sketchy. I largely copied it from the interface converter.

@hol430 hol430 force-pushed the interface-arrays branch 2 times, most recently from 424d2f2 to eb7d6a5 Compare March 30, 2023 03:32
Copy link
Member

@badcel badcel left a comment

Choose a reason for hiding this comment

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

Thank you for PR. In general this looks okayish to me. But this is not due to your code, but because I just discovered that there is potentially a pretty big bug (see #840).

Can you test if a method with an interface glib pointer array return type works even if some memory is leaked?

If not I think we should probably add a unit test to verify the behavior or go in the opposite direction not rendering any function returning a GLibPtrArray of interfaces. This could be done by keeping the files you added and just throw an exception in the "Convert" method. If the generator crashes after this change some "try/catch" blocks are missing: An exception in some converter should not stop the generator but just stop generation of the current element.

The build pipeline reported there is something wrong with your file encodings. Can you fix those, too?

@@ -0,0 +1,14 @@
namespace Generator.Renderer.Internal.ReturnType;

internal class InterfaceArray : ReturnTypeConverter
Copy link
Member

Choose a reason for hiding this comment

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

As this class handles "GLibPtrArray" please name it InterfaceGLibPtrArray instead of InterfaceArray. We need to keep this name available for the regular array implementations.


namespace Generator.Renderer.Public.ReturnType;

internal class InterfaceArray : ReturnTypeConverter
Copy link
Member

Choose a reason for hiding this comment

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

As this class handles "GLibPtrArray" please name it InterfaceGLibPtrArray instead of InterfaceArray. We need to keep this name available for the regular array implementations.


namespace Generator.Renderer.Public.ReturnTypeToManagedExpressions;

internal class InterfaceArray : ReturnTypeConverter
Copy link
Member

Choose a reason for hiding this comment

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

As this class handles "GLibPtrArray" please name it InterfaceGLibPtrArray instead of InterfaceArray. We need to keep this name available for the regular array implementations.

@badcel
Copy link
Member

badcel commented Mar 30, 2023

Hm, I think the code can't work. See the docs. The array is just a struct of a pointer and a length. Treating it as a regular array reads memory it probably shouldn't or can't read at all.

Perhaps we should really go in the opposite direction and disable those functions. This should allow adding GtkSourceView and then we add support for GLib arrays as part of #840.

It can even be that there are already functions generated and be broken in a similar way if they use GLib arrays. I think I don't have a proper solution for them currently and we need to wait for #840.

@cameronwhite: Do you agree?

@cameronwhite
Copy link
Contributor

If I'm reading these changes correctly, the internal return type currently would be IntPtr[]?
I agree that is incorrect and would probably result in a crash because it's not matching the C return type (which I think would be a GPtrArray *)

I see there are generated Glib.Internal.PtrArray*Handle classes which would be a correct return type. But I don't think those are quite usable yet since there isn't access to the struct members, and the owned handle doesn't know the correct free() function to use (related to #622)

@badcel
Copy link
Member

badcel commented Mar 31, 2023

Yes it would be IntPtr[].

@hol430 So let's do the opposite of your PR then. Meaning the PR ensures that nothing related to GLib arrays is rendered.

@hol430 hol430 force-pushed the interface-arrays branch from eb7d6a5 to 34d966e Compare April 17, 2023 00:09
@hol430 hol430 force-pushed the interface-arrays branch from 34d966e to 6a614ef Compare April 20, 2023 03:45
@hol430 hol430 changed the title Support interface arrays Slightly improved support for GPtrArray Apr 20, 2023
@hol430
Copy link
Contributor Author

hol430 commented Apr 20, 2023

@badcel looks like totally preventing any GPtrArrays from being rendered is also problematic; there's a delegate in Gio which takes a parameter of type GPtrArray*; if we remove support for this parameter, the delegate doesn't get rendered but a callback which relies on the delegate still gets rendered, which results in broken bindings (same problem as we discussed the other day). What I've done here instead is to render GPtrArray in the internal functions as IntPtr which should actually work (previously it was IntPtr[]). Let me know if you have any better ideas.

@badcel
Copy link
Member

badcel commented Apr 20, 2023

Looks good to me. Thank you 👍

@badcel badcel merged commit 92429d6 into gircore:main Apr 20, 2023
@hol430 hol430 deleted the interface-arrays branch April 23, 2023 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants