-
Notifications
You must be signed in to change notification settings - Fork 88
[ffigen] Generate Dart constants for static const C variables #2854
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: main
Are you sure you want to change the base?
Conversation
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
|
Thanks for your contribution! The overall approach looks good to me. In your bug, you mentioned that we're generating setters for these variables. It seems like this PR doesn't address that issue (which is fine, that can be fixed in a separate PR). I see we're already plumbing through I haven't looked into why supposedly const
That's fine. The next release of FFIgen (v21) is a breaking change anyway, and this is clearly an improvement.
Proxying whenever possible seems like a good thing to me. If a string can be proxied, it probably should be, since this improves readability and is also a small performance win. |
|
@liamappelbe Thank for reviewing! outdatedI actually just created a simple test linked in #2816 (comment) which I think shows that this would be needed for static const strings as well so I'll re-open this draft.
Well, it does fix it in that now there are no getter or setters, just a dart value. (edit: or maybe not, let me check if it actually worked...)
I'm not sure either. I was getting this when creating bindings to Dawn. Everything else worked and I can call into Dawn fine, just can't use these static consts.
Yes, that make sense, although I don't know how to repro or diagnose the bug.
Ok. I will try re-running the bindings on dawn with my fork to see if the bug goes away. And I will try these debug prints. edit: I re-ran ffigen and indeed now it creates usable bindings: So I'm not sure about how that works. I just remembered though that I reproed this issue without Dawn, using just this header: #define MY_MACRO_CONST 100
int my_global_var;
const int my_const_var;
static int my_static_var = 200;
static const int my_static_const_var = 300;
So now I'll try to repro that again. Ok, turns out the setter for static const vars was only happening with a very old version. (I had been using version 10.) So that's probably why. I should have looked at the version before opening that bug or forking the repo :) I think this PR is still needed to be able to use static const values though.. |
|
Turns out that broken test should not have been broken, it was opted in to get the memory address for that string like so: globals:
symbol-address:
include:
- library_versionlatest commit should fix that. |
liamappelbe
left a comment
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.
Looks good. Just a few minor formatting issues.
| - __Breaking change__: Certain synthetic USRs have been modified to ensure they | ||
| cannot collide with real USRs. It's very unlikely that any user facing USRs | ||
| are affected. | ||
| - __Breaking change__: Dart const values will be generated for global variables marked const in C (e.g. static const int) instead of symbol lookups. This supports integers, doubles, and string literals. Including the variable name in the globals -> symbol-address configuration will still generate symbol lookups. |
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.
nit: Use an 80 column wrap.
| entry-points: | ||
| - static_const.h | ||
| include-directives: | ||
| - '**static_const.h' No newline at end of file |
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.
nit: Add a trailing newline
|
|
||
| // Should be generated as Globals. | ||
| static const char TEST_STRING_ARRAY[] = "test_array"; | ||
| int test_global; No newline at end of file |
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.
nit: Add a trailing newline
fixes #2816.
This allows headers like this to work:
source: https://github.com/webgpu-native/webgpu-headers/blob/12c1d34e7464cac58cc41a24aeee1d48a2f21b74/webgpu.h#L1266
Currently the generated code looks like this:
But this doesn't work for me and also seems incorrect (creating a setter for a static const?). It seems that static const variables are not included in the lookup table, at least when I tried to access it, it failed for me. See the linked issue for more explanation.
I moved _getWrittenRepresentation and _writeDoubleAsString to utils to make them public. I also changed
Global? parseVarDeclarationtoBinding? parseVarDeclaration.I'm opening this PR in the hopes of getting some feedback on if this is the right way to solve the issue. It seems to me to be correct but I'm also not familiar with the codebase.
Also, this is a breaking change. RIght now this test is failing:
native/pkgs/ffigen/example/ffinative/headers/example.h
Lines 24 to 25 in 9d538ae
which used to genrate
native/pkgs/ffigen/example/ffinative/lib/generated_bindings.dart
Lines 42 to 44 in 9d538ae
but now is
edit: I should have looked into this more before opening the PR, but it also seems that maybe this issue only happens with integral types, so maybe this isn't needed for Strings, in which case I should remove that. Marking as draft for now, but if anyone has any thought or suggestions please let me know.
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.