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

Add more comprehensive support for primitive value type parameters #795

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

cameronwhite
Copy link
Contributor

I think this covers all of the possible cases for primitive value types, with the main addition being support for functions with inout or out primitive value parameters

  • Don't generate public bindings for primitive value pointer types that have a direction of in. Many occurrences of this in the gir files were actually treating the pointer as an array, but didn't mark it as such in the documentation (e.g. gdk_pango_layout_get_clip_region()), so I don't think we can safely generate public bindings for this case.

  • Translate inout and out parameters to 'ref' and 'out' parameters in C#.

    • Nullable out parameters are just exposed as non-nullable out parameters in C#. In C the meaning of null is "ignore the output value", so in C# I think the idiomatic equivalent is to just have the user do out var _ if they want to ignore it. An out int? parameter doesn't really make sense with what the native function is doing.
    • Nullable inout parameters are similarly exposed as a non-nullable ref parameter. I'm less sure about this - it could perhaps be a ref int? parameter, but this case only occurs for gst_init() currently.
    • The caller-allocates and transfer flags seem to be meaningless and are inconsistently used by the native functions. I didn't find any occurrences of native functions allocating or freeing a primitive value type pointer
  • With these changes, Pango.Layout.GetLogAttrs() was now being generated but encountered a compile error with record arrays that have direction=out, since the generated code doesn't work at all in that case. This is skipped for now, to be handled with bug Verify handling of out and inout array parameters #763

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.

This is some hard topic. Let's start with some trivial fixes which are my comments left on the code.

Before getting into the details let me state that I did not take a detailed look into the code. But this was not strictly necessary as you probably missed one point:

Out / InOut parameters can return null and thus be nullable. The use case use case you are describing is called optional.

See the GI documentation for further explanation.

In general the generator currently does not know about the optional attribute. But I assume it is not needed currently. Or at least could be part of another PR?

I created a new branch based on yours which updates the unit tests in a way that I think we need to be able to prove that inout / out parameters are working correctly.

What do you think?

@cameronwhite cameronwhite force-pushed the feature/inout-primitive-value-type branch from 74c0e65 to 8a4a5ef Compare February 9, 2023 03:39
@cameronwhite cameronwhite force-pushed the feature/inout-primitive-value-type branch 2 times, most recently from b0f7e8c to 7c300f1 Compare February 9, 2023 03:46
@cameronwhite
Copy link
Contributor Author

Updated with the style changes, and to read the optional attribute from the gir files (it was pretty simple, but I can move that to a separate PR if that's easier)

Parameters that are both nullable and optional are skipped since this would require extra handling, and this doesn't actually occur for any functions currently.
Parameters that are only nullable are still allowed, although they should likely have their annotations fixed to use optional. I've left these enabled since there are some required functions like gst_init() that do this.

….g. directions of inout or out

- Don't generate public bindings for primitive value pointer types that have a direction of 'in'. The occurrences of this in the gir files were all actually treating the pointer as an array, but didn't mark it as such in the documentation (e.g. gdk_pango_layout_get_clip_region)

- Translate inout and out parameters to 'ref' and 'out' parameters in C#.
  - Optional out parameters are just exposed as (non-nullable) out parameters in C#. In C the meaning of this is "ignore the output value", so in C# I think the idiomatic equivalent is to just have the user do `out var _` if they want to ignore it.
  - Optional inout parameters are similarly exposed as a non-nullable ref parameter for simplicity
  - The caller-allocates and transfer flags should not be used for primitive types (https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/446)

- With these changes, Pango.Layout.GetLogAttrs() is now generated but encountered an error with record arrays that have direction=out. This is skipped for now, to be handled with bug #763
@cameronwhite cameronwhite force-pushed the feature/inout-primitive-value-type branch from 7c300f1 to a0b3103 Compare February 9, 2023 23:02
@badcel badcel merged commit fdc945f into main Feb 10, 2023
@badcel badcel deleted the feature/inout-primitive-value-type branch February 10, 2023 11:20
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.

2 participants