-
Notifications
You must be signed in to change notification settings - Fork 103
[jnigen] filter out Kotlin DefaultConstructorMarker from Dart bindings #3048
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?
[jnigen] filter out Kotlin DefaultConstructorMarker from Dart bindings #3048
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 |
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.
This looks reasonable, but it needs a test. You've added a test DefaultParams.kt file, but you're not doing anything with it. You'll need to regenerate the kotlin_test bindings using test/kotlin_test/generate.dart, and then verify the behavior of the bindings by adding some tests to test/kotlin_test/runtime_test_registrant.dart
d59ab09 to
2d8609e
Compare
[jnigen] Add runtime tests for Kotlin DefaultConstructorMarker filtering Revert kotlin.dart to previous state
885e19c to
64fb1ef
Compare
|
Please don't force push unless you absolutely need to (I think I've had to force push once in the last 5 years). It deletes the commit history which makes your PR way harder to review. |
|
You still haven't updated the test's generated code, which is probably why the analyzer bot is failing |
|
Hi @liamappelbe — thanks for the clarification, and sorry about the force-push earlier. That’s on me, I’ll avoid doing that going forward. I’ve now regenerated the Let me know if there’s anything else you’d like me to adjust or verify. |
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.
Also, take a look at the analysis errors: https://github.com/dart-lang/native/actions/runs/21744051314/job/63028164140?pr=3048
| jni$_.JString? string, | ||
| core$_.bool z, | ||
| int i1, | ||
| jni$_.JObject? defaultConstructorMarker, |
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 like there's still some markers in the generated code.
What this does:
This updates jnigen to handle Kotlin default constructors properly.
Kotlin generates a synthetic DefaultConstructorMarker parameter for constructors with default values. That parameter shouldn’t show up in the generated Dart API. This change hides it from Dart, and always passes null to the JNI constructor instead, which is what Kotlin expects anyway.
What changed
This only applies to Kotlin-generated synthetic constructors and does not affect normal Java constructors.
Fixes: #1986
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.