[jnigen] Clean up .reference.pointer pattern#3350
Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with
API leaks
|
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| jni | $JIterator | core_bindings.dart::JIterator::implementIn::$impl core_bindings.dart::JIterator::implement::$impl |
| jni | $JCollection | core_bindings.dart::JCollection::implementIn::$impl core_bindings.dart::JCollection::implement::$impl |
| jni | $JList | core_bindings.dart::JList::implementIn::$impl core_bindings.dart::JList::implement::$impl |
| jni | $JMap$JEntry | core_bindings.dart::JMap$JEntry::implementIn::$impl core_bindings.dart::JMap$JEntry::implement::$impl |
| jni | $JMap | core_bindings.dart::JMap::implementIn::$impl core_bindings.dart::JMap::implement::$impl |
| jni | $JSet | core_bindings.dart::JSet::implementIn::$impl core_bindings.dart::JSet::implement::$impl |
This check can be disabled by tagging the PR with skip-leaking-check.
| return _new$(_class.reference.pointer, _id_new$.pointer) | ||
| .object<Notifications>(); | ||
| final _$$classRef = _class.reference; | ||
| return _new$(_$$classRef.pointer, _id_new$.pointer).object<Notifications>(); |
There was a problem hiding this comment.
The before and after code looks semantically identical. Why do we need to rewrite these?
There was a problem hiding this comment.
Is this a work around for a bug that should ideally be fixed in the dart SDK (e.g. what Daco is also saying in #3342 (comment)?)?
If so, do we have an upstream issue that we could link in a TODO in dart_generator.dart to make it more explicit why we pull the reference out into a variable?
There was a problem hiding this comment.
Yeah, it's a bug in FinalizableTransformer (filed one). We should fix it, but I'm not sure how hard it'll be to fix. In any case we'll need this work around in the short term.
2 participants
For the most part, JNIgen was already storing all
foo.reference.pointers in a local variable. There were just a few remaining cases: the class reference for static methods, the self reference for non-static methods, and one other random case in kotlin suspend functions. Not sure if these cases can hit the same GC issue, but I'm switching them to locals just to be safe.Fixes #3346