[ffigen] Add retainOwnership/releaseOwnership to C++ class bindings#3459
[ffigen] Add retainOwnership/releaseOwnership to C++ class bindings#3459Hassnaa9 wants to merge 2 commits into
Conversation
Adds two new methods to every generated C++ Dart wrapper class: - retainOwnership([customFinalizer]): attaches a NativeFinalizer to this object, taking ownership of the underlying C++ pointer. An optional native function pointer allows specifying a custom deallocator (e.g. free instead of delete). - releaseOwnership(): detaches the finalizer, transferring ownership responsibility to the caller. These are the escape hatches described as Option 1 in the memory management design doc. They allow developers to correctly handle raw C++ pointer APIs whose ownership semantics cannot be inferred from the function signature alone. The dispose() method now delegates its detach call to releaseOwnership() for consistency. A per-instance _activeFinalizer field tracks whether the object currently owns its pointer (null = unowned). Tests cover: - releaseOwnership suppresses the GC-triggered finalizer - retainOwnership after releaseOwnership re-enables the GC finalizer - Both methods are idempotent
PR HealthBreaking changes ✔️
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 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
liamappelbe
left a comment
There was a problem hiding this comment.
Looks pretty good overall. I think we can probably simplify a little bit though. You've got 3 fields in the wrapper object: _ptr, _isDisposed, and _activeFinalizer.
The states the object can be in are:
| State | _ptr |
_activeFinalizer |
_isDisposed |
|---|---|---|---|
| Alive and owned | non-null | non-null | false |
| Alive and unowned | non-null | null | false |
| Disposed | invalid | invalid | true |
I think that's it? Are there any states I missed?
If that's all the states, then we can probably get rid of _isDisposed and just set both _ptr and _activeFinalizer to null.
| @@ -120,14 +120,48 @@ class $name implements $ffiPrefix.Finalizable { | |||
| s.write(''' | |||
| bool _isDisposed = false; | |||
| static final _finalizer = $ffiPrefix.NativeFinalizer( | |||
There was a problem hiding this comment.
For clarity, call this _defaultFinalizer
| /// default `delete` finalizer, which is useful when the object was not | ||
| /// allocated with `new` (e.g. `malloc` or a custom allocator). | ||
| /// | ||
| /// Has no effect if this object already owns the pointer. |
There was a problem hiding this comment.
Maybe this should throw a StateError?
| /// underlying C++ pointer. The caller becomes responsible for freeing | ||
| /// the memory. | ||
| /// | ||
| /// Has no effect if this object does not own the pointer. |
There was a problem hiding this comment.
This should probably also throw a StateError
| _isDisposed = true; | ||
| _finalizer.detach(this); | ||
| releaseOwnership(); | ||
| $deleteGlue(_ptr); |
There was a problem hiding this comment.
What if they used a custom finalizer instead of deleteGlue?
There was a problem hiding this comment.
I think I've addressed the issue, but I still feel there may be some improvements to make, especially around the test coverage. I'll think through a few more scenarios and update them if I find any gaps.
2 participants
#3450