Fix no return static analysis error in SchedulerPriorityUtils.h#47911
Closed
acoates-ms wants to merge 5 commits into
Closed
Fix no return static analysis error in SchedulerPriorityUtils.h#47911acoates-ms wants to merge 5 commits into
acoates-ms wants to merge 5 commits into
Conversation
facebook-github-bot
added
CLA Signed
facebook-github-bot
added
the
Shared with Meta
| case SchedulerPriority::IdlePriority: | ||
| return std::chrono::minutes(5); | ||
| default: | ||
| react_native_assert(false && "Unsupported SchedulerPriority value"); |
Contributor
There was a problem hiding this comment.
Nit: could you place the return outside the switch/case? Otherwise this suppresses any Clang warnings for switch/case exhaustiveness if a new enum value is added.
Contributor
There was a problem hiding this comment.
We also do LOG(FATAL) in some places which is marked as [[noreturn]], and will run in release builds, but either should be fine.
NickGerleman
reviewed
Dec 9, 2024
| return std::chrono::seconds(10); | ||
| case SchedulerPriority::IdlePriority: | ||
| return std::chrono::minutes(5); | ||
| default: |
Contributor
There was a problem hiding this comment.
I wasn’t very clear here.
We build with Clang “-Wswitch-enum”, which will create a warning/error if switch case does not handle a specific enum value.
That means if we add a new SchedulerPriority, we get a build error to tell us where we need to handle the new case. Unless we have a “default”. Having a default satisfies exhaustiveness, but means that adding support for a new enum value is now a runtime error instead of a compile error.
So, we want to avoid default cases, in any switch where we want new enum values to be handled.
But we can return a value, or fatal, after the switch/case, to satisfy MSVC, and let it know that the function will either return a value, or throw/terminate.
Contributor
Author
There was a problem hiding this comment.
Yup, that makes sense. I didn't really think about it, and just did a search for LOG(FATAL) was used in switches elsewhere. This is the one I "copied":
I guess that should probably also be updated to avoid the default, but I'll leave that outside the scope of this change.
Contributor
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
NickGerleman
reviewed
Dec 9, 2024
| react_native_assert(false && "Unsupported SchedulerPriority value"); | ||
| return SchedulerPriority::NormalPriority; | ||
| } | ||
| LOG(FATAL) << "Unknown SchedulerPriority"; |
Contributor
There was a problem hiding this comment.
Theoretically this should be [[noreturn]] (RNW Glog stub has dtor be noreturn?)
But not sure MSVC likes that I guess? https://github.com/google/glog/blob/6c5c692c8e423f651c74de9477ff0b5a59008bcc/src/striplog_unittest.cc#L53
Does RNW require the return after this?
Contributor
|
iOS CI failures are from this diff. Feel free to just use react_native_assert if it's easier. Likely missing glog dep from podspec. CocoaPods logic is a bit messy, so it might just be a missing dep, or might be more complicated. |
Contributor
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
|
@NickGerleman merged this pull request in 849407a. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Dec 12, 2024
Summary: `react_native_assert` on iOS uses glog under the hood, and #47911 added usage to a new podspec, which means new entire binary under some build modes. Need to add missing dependency I think? Changelog: [Internal] Pull Request resolved: #48241 Test Plan: tes_ios_helloworld passes with DynamicLibraries Reviewed By: cipolleschi Differential Revision: D67141052 Pulled By: NickGerleman fbshipit-source-id: 299a499f40e9b54c4aca5d6e1c95c43ce933fb2b
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary:
In react-native-windows our static analysis tools report an error for
timeoutForSchedulerPrioritydue to cases where it may not always return a value. This is an upstreaming of the patch we have to fix that error.Changelog:
[INTERNAL] [FIXED] - Fix no return static analysis error in SchedulerPriorityUtils.h
Test Plan:
Building should be sufficient.