This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Roll buildroot to pick up disable NSAsserts in release builds #53005
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmagman
changed the title
bc-lee
added a commit
to bc-lee/flutter-engine
that referenced
this pull request
May 26, 2024
In flutter/buildroot#860 , disabling NSAssert in release build was added, and flutter#53005 tries to roll the change into the engine. However, the change is blocked due to several compilation errors in the engine. This is due to the fact that Google's DCHECK(in flutter engine, FML_DCHECK) does not make unused variable warning even in release build, while traditional assertion macro like assert or NSAssert does. For example, the following code will be expanded like this: code: FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer"; expanded code: true || (status == kCVReturnSuccess && pxbuffer != nullptr) ? (void)0 :: : fml::LogMessageVoidify() & ::fml::LogMessage(::fml::kLogFatal, 0, 0, nullptr).stream() However, equivalent code with NSAssert will make unused variable warning, since the expanded code doesn't have any reference to the variable: code: NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer."); expanded code: do { } while (0) To unblock the build, this CL replaces several NSAssert with FML_DCHECK in the engine codebase. This CL isn't aimed to replace all NSAssert with FML_DCHECK since discussions in flutter/buildroot#860 are not finalized whether to replace all NSAssert with FML_DCHECK or not.
8 tasks
|
From PR review triage: If it's not possible to roll a buildroot change into the engine as a fast follow-on, then the best practice would be to revert the buildroot change in case other changes to the buildroot need to be rolled in. |
auto-submit bot
pushed a commit
that referenced
this pull request
May 30, 2024
…53048) In flutter/buildroot#860, disabling `NSAssert` in release build was added, and #53005 tries to roll the change into the engine. However, the change is blocked due to several compilation errors in the engine. This is due to the fact that Google's `DCHECK`(in flutter engine, `FML_DCHECK`) does not make unused variable warning even in release build, while traditional assertion macro like `assert` or `NSAssert` does. For example, the following code will be expanded like this: code: ```c++ FML_DCHECK(status == kCVReturnSuccess && pxbuffer != nullptr) << "Failed to create pixel buffer"; ``` expanded code: ```c++ true || (status == kCVReturnSuccess && pxbuffer != nullptr) ? (void)0 :: : fml::LogMessageVoidify() & ::fml::LogMessage(::fml::kLogFatal, 0, 0, nullptr).stream() ``` However, equivalent code with `NSAssert` will make unused variable warning, since the expanded code doesn't have any reference to the variable: code: ```c++ NSAssert(status == kCVReturnSuccess && pxbuffer != NULL, @"Failed to create pixel buffer."); ``` expanded code: ```c++ do { } while (0) ``` To unblock the build, this CL replaces several `NSAssert` with `FML_DCHECK` in the engine codebase. This CL isn't aimed to replace all `NSAssert` with `FML_DCHECK` since discussions in flutter/buildroot#860 are not finalized whether to replace all `NSAssert` with `FML_DCHECK` or not. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
|
Roger, will do that next time. |
Still failing: |
|
Closing for now. flutter/buildroot#864 |
8 tasks
auto-submit bot
pushed a commit
that referenced
this pull request
Nov 15, 2024
If we disable the NSAssert during release mode, this code will continue after NSAssert failure, which will then pass a `nil` value into a dictionary (which cannot have a nil value), hence the clang-tidy warning here #53005 (comment) *List which issues are fixed by this PR. You must list at least one issue.* flutter/flutter#148279 flutter/flutter#157837 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822
pushed a commit
to nick9822/flutter
that referenced
this pull request
Dec 18, 2024
…6588) If we disable the NSAssert during release mode, this code will continue after NSAssert failure, which will then pass a `nil` value into a dictionary (which cannot have a nil value), hence the clang-tidy warning here flutter/engine#53005 (comment) *List which issues are fixed by this PR. You must list at least one issue.* flutter#148279 flutter#157837 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Roll flutter/buildroot#860 "Disable NSAsserts in release builds"
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.