-
Notifications
You must be signed in to change notification settings - Fork 29.7k
iOS: Change engine NSAssert to FML_CHECK #166009
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
iOS: Change engine NSAssert to FML_CHECK #166009
Conversation
We assume the engine is non-nil everywhere in the engine, further, this
is a one-time check with near-zero runtime performance -- we're not
creating view controllers at any frequency that will cause a nil check
to cause any effect. This also guarantees that the device logs an error
message that makes clear exactly what happened, even in release builds.
This guarantees that both in debug and release builds, we get useful
logging if a nil engine is passed to the initializer.
The initializer *does* declare the FlutterEngine* parameter to be
non-nil, but this only provides a hint to the compiler and doesn't
prevent nil engines being passed at runtime, or really at compile time;
in our clang toolchain, the following compiles fine:
```objc
FlutterEngine* engine = nil;
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
```
Long-term, we should resolve
flutter#157837 and migrate from
FML_CHECK and FML_DCHECK, to using NSAssert agressively in the embedder.
No test changes since this introduces no changes to the debug-mode
builds used in testing.
Issue: flutter#153971
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
test-exempt: Semantically similar, used for debug-mode only. |
We assume the engine is non-nil everywhere in the engine, further, this
is a one-time check with near-zero runtime performance -- we're not
creating view controllers at any frequency that will cause a nil check
to cause any impact. This also guarantees that the device logs an error
message that makes clear exactly what happened, even in release builds.
This guarantees that both in debug and release builds, we get useful
logging if a nil engine is passed to the initializer.
The initializer *does* declare the FlutterEngine* parameter to be
non-nil, but this only provides a hint to the compiler and doesn't
prevent nil engines being passed at runtime, or really at compile time;
in our clang toolchain, the following compiles fine:
```objc
FlutterEngine* engine = nil;
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
```
Long-term, we should resolve
flutter#157837 and migrate from
FML_CHECK and FML_DCHECK, to using NSAssert agressively in the embedder.
For further details, see my comments here:
flutter#153971 (comment)
No test changes since this introduces no changes to the debug-mode
builds used in testing.
Fixes: flutter#153971
## Pre-launch Checklist
- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [X] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
We assume the engine is non-nil everywhere in the engine, further, this
is a one-time check with near-zero runtime performance -- we're not
creating view controllers at any frequency that will cause a nil check
to cause any impact. This also guarantees that the device logs an error
message that makes clear exactly what happened, even in release builds.
This guarantees that both in debug and release builds, we get useful
logging if a nil engine is passed to the initializer.
The initializer *does* declare the FlutterEngine* parameter to be
non-nil, but this only provides a hint to the compiler and doesn't
prevent nil engines being passed at runtime, or really at compile time;
in our clang toolchain, the following compiles fine:
```objc
FlutterEngine* engine = nil;
FlutterViewController* viewController = [[FlutterViewController alloc] initWithEngine:engine
nibName:nil
bundle:nil];
```
Long-term, we should resolve
flutter#157837 and migrate from
FML_CHECK and FML_DCHECK, to using NSAssert agressively in the embedder.
For further details, see my comments here:
flutter#153971 (comment)
No test changes since this introduces no changes to the debug-mode
builds used in testing.
Fixes: flutter#153971
## Pre-launch Checklist
- [X] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [X] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [X] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [X] I signed the [CLA].
- [X] I listed at least one issue that this PR fixes in the description
above.
- [X] I updated/added relevant documentation (doc comments with `///`).
- [X] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [X] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel
on [Discord].
<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
We assume the engine is non-nil everywhere in the engine, further, this is a one-time check with near-zero runtime performance -- we're not creating view controllers at any frequency that will cause a nil check to cause any impact. This also guarantees that the device logs an error message that makes clear exactly what happened, even in release builds.
This guarantees that both in debug and release builds, we get useful logging if a nil engine is passed to the initializer.
The initializer does declare the FlutterEngine* parameter to be non-nil, but this only provides a hint to the compiler and doesn't prevent nil engines being passed at runtime, or really at compile time; in our clang toolchain, the following compiles fine:
Long-term, we should resolve #157837 and migrate from FML_CHECK and FML_DCHECK, to using NSAssert agressively in the embedder.
For further details, see my comments here:
#153971 (comment)
No test changes since this introduces no changes to the debug-mode builds used in testing.
Fixes: #153971
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.