-
Couldn't load subscription status.
- Fork 84
7903947: Access to function pointers in structs could be streamlined #287
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
base: master
Are you sure you want to change the base?
7903947: Access to function pointers in structs could be streamlined #287
Conversation
|
👋 Welcome back nbenalla! A progress list of the required criteria for merging this PR into |
|
@nizarbenalla This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
Just to recap solution we have explored previously. It would be nice if, in principle, just doing this: worked, and it invoked the correct function pointer on the Another solution would be to tweak (note also that we cannot "just" add an overload of In other words, there's no "easy" solution here. One possibility might be to add an invoker accessor to The other solution is the one explored here: the developer knows this isn't just any struct, but a "functional" struct. It tells jextract, and jextract reacts by generating slightly different code. Perhaps one issue with this approach (although we arrived at it honestly) is that if one stares at code like: It is pretty difficult to tell whether that's a getter/setter or an "invoker". |
| /** | ||
| * Generates exactly one invoker, inlining the struct.get(...) to avoid name collision. | ||
| */ | ||
| private void emitFunctionalConvenience(String invokerName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this emitInvoker
| } else if (Utils.isPointer(type) || Utils.isPrimitive(type)) { | ||
| boolean isFuncPtr = functionalDispatch && Utils.isFunctionPointer(type); | ||
| if (isFuncPtr) { | ||
| emitFieldSetter(javaName, varTree, layoutField, offsetField); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we even want the setter -- the fields in these structs are typically populated by a library
| } | ||
| } | ||
|
|
||
| if (kind == IncludeKind.STRUCT || kind == IncludeKind.UNION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I get this logic -- it seems like we're always setting a property P for a typedef with name N whenever we see it set for a struct N? That seems odd.
But in example as the one you added in the test are a bit ambiguous, as the same declaration induces both an anonymous struct decl and a typedef, so it is less clear which is which. If I write:
typedef struct Foo {
int a;
};
It seems to be that jextract only retains a "struct" tree, and throws away the typedef. In fact I was surprised that your test worked at all, as in the above case dump-includes would include something like this:
--include-struct Foo # header: ....
And not:
--include-typedef Foo # header: ....
So adding a property on the typedef should have no effect?
|
Mailing list message from Duncan Gittins on jextract-dev: This proposal makes it nicer to handle invokers. I generated my code with // public static void Release(MemorySegment struct, MemorySegment Will the use of ",functional" be the intended way to trigger this Kind regards Duncan On Thu, 24 Jul 2025 at 10:29, Maurizio Cimadamore <mcimadamore at openjdk.org> -------------- next part -------------- |
Nice connection -- we do in fact have plans (at least in theory) to use this same mechanism for errno -- and maybe even to declare "disjoint enums" (e.g. enums that can be modelled as plain Java enums because you swear not to rely on low-level bitmask operation -- which is often the case if the enum you are modelling is just a "tag") |
I think the issue is that the changes in this PR generate both setter and invoker -- and we should only emit the invoker. |
|
Mailing list message from Duncan Gittins on jextract-dev: Now I think about it, renaming the invoker may be necessary after all. Duncan On Thu, 24 Jul 2025 at 16:05, Maurizio Cimadamore <mcimadamore at openjdk.org> -------------- next part -------------- |
Thanks, this is an useful data point. If it turns out that our assumption that "these structs only need invokers" is not 100% true, that doesn't leave us in a good place. While you could refrain from using the new flag on your struct, it feels like a "glass half empty" situation, because the code jextract would be able to generate is almost there. Which brings us back to another option: maybe the more honest option is to indeed add an invoker-like accessor for each function pointer field (e.g. Foo.m$invoke). The dollar in the same sucks a bit but, oh well, at least it's clear. Then, let's approach this from the other angle, and give developer more options to NOT generate stuff they don't want. E.g. maybe this struct is read-only and doesn't need setters? Or maybe it's invoke-only and doesn't need neither getters nor setters? Whatever the needs, and whatever the reason, we'd have a way to generate, more or less, only the stuff one would be interested in. So, maybe, instead of "functional struct", maybe some way to say what you want in a struct -- similar to Linux filesystem |
|
I kept trying to avoid renaming the invoker as I didn't like having the dollar sign or a prefix in the invoker, but I like the idea of having readonly/write/execute mode in the config. |
I feel your pain. We defo went thorugh a lot of pain to remove dollars from generated code: https://cr.openjdk.org/~mcimadamore/panama/jextract_changes.html So, it will be a bit sad to see them coming back. Also, it will make some code more verbose compared to their C counterpart, as the C syntax can distinguish between application ( |
Maybe worth thinking some more about what our options are... |
|
@nizarbenalla This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
Keep Alive |
|
@nizarbenalla This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Please review this patch to add a new option where you can generate an direct invoker methods. This feature is intended to be used on structs that are just a collection of functions we had like to call.
If this feature is used, we remove the getter to avoid name collisions as we assume this is similar to an interface.
Changes to
GUIDE.mdhave been intentionally left out from this initial patch to make reviews easier.Before
After
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jextract.git pull/287/head:pull/287$ git checkout pull/287Update a local copy of the PR:
$ git checkout pull/287$ git pull https://git.openjdk.org/jextract.git pull/287/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 287View PR using the GUI difftool:
$ git pr show -t 287Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jextract/pull/287.diff
Using Webrev
Link to Webrev Comment