[iOS] Eliminate ARC __bridge casts where possible #155943
Assignees
Labels
c: tech-debt
Technical debt, code quality, testing, etc.
engine
flutter/engine repository. See also e: labels.
P2
Important issues not at the top of the work list
team-ios
Owned by iOS platform team
triaged-ios
Triaged by iOS platform team
Comments
cbracken
added
P2
|
Following the link from https://github.com/flutter/engine/blob/033fa28a58bb775a519dc5fde148d5b4dbee3b20/shell/platform/darwin/ios/framework/Source/FlutterEngineGroup.mm#L86-L87 |
|
Yep - I was looking at doing just that; the one issue with NSPointerArray is that it doesn't have a remove by value message, just by index. Probably still worth it though. |
4 tasks
jmagman
added
triaged-ios
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 16, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
8 tasks
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 16, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 16, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 17, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 17, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon dealloc. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must must contain a nil pointer and can thus skip compaction. When holding weak pointers under ARC, this is no longer the case and so we work around the issue by storing a nil pointer before calling compact. See: http://www.openradar.me/15396578 I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943
auto-submit bot
pushed a commit
to flutter/engine
that referenced
this issue
Nov 17, 2024
FlutterEngineGroup keeps an array of all live FlutterEngine instances it has created such that after the first engine, it can spawn further engines using the first. Previously we manually managed this array by adding engines to it upon creation, then having [FlutterEngine dealloc] emit a notification such that FlutterEngineGroup can listen for it, and remove instances from the array upon reception of the notification. Instead, we now use an NSPointerArray of of weak pointers such that pointers are automatically nilled out by ARC after the last strong reference is collected. This eliminates the need for the manual notification during dealloc. Unfortunately, NSPointerArray is "clever" and assumes that if the array has not been mutated to store a nil pointer since its last compact call, it must not contain a nil pointer and can thus skip compaction. Under ARC, weak pointers are automatically nilled out when the last strong reference is released, so the above heuristic is no longer valid. We work around the issue by storing a nil pointer before calling compact. See http://www.openradar.me/15396578 for the radar tracking this bug. I'm not thrilled with the fact that we're replacing one sort of TODO with another, but the code is much simpler and avoids relying on a trip through the notification center, which seems objectively better. Issue: flutter/flutter#155943 Issue: flutter/flutter#137801 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
cbracken
added a commit
to cbracken/flutter_engine
that referenced
this issue
Nov 17, 2024
This replaces a few ARC bridged retain casts to regular bridge casts to CoreFoundation types, which are then CFRetained via `sk_cfp::retain` calls. This eliminates the last remaining unnecessary __bridge_retained casts in the codebase. The remaining casts have been manually vetted and are reasonable. Issue: flutter/flutter#155943
8 tasks
auto-submit bot
pushed a commit
to flutter/engine
that referenced
this issue
Nov 17, 2024
This replaces a few ARC bridged retain casts to regular bridge casts to CoreFoundation types, which are then CFRetained via `sk_cfp::retain` calls. This eliminates the last remaining unnecessary __bridge_retained casts in the codebase. The remaining casts have been manually vetted and are reasonable. Issue: flutter/flutter#155943 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
|
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
During ARC migration in #137801 we left behind some unfortunate
__bridgecasts. In the interest of safety and readability, we should eliminate these where possible.The text was updated successfully, but these errors were encountered: