-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764
Conversation
@@ -173,7 +173,7 @@ - (NSUInteger)hash { | |||
} | |||
@end | |||
|
|||
NSObject const* FlutterMethodNotImplemented = [NSObject new]; | |||
NSObject const* FlutterMethodNotImplemented = [[NSObject alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-objc-avoid-nsobject-new
@@ -6,7 +6,7 @@ | |||
|
|||
#include "gtest/gtest.h" | |||
|
|||
void checkEncodeDecode(id value, NSData* expectedEncoding) { | |||
static void CheckEncodeDecode(id value, NSData* expectedEncoding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-objc-function-naming
@@ -21,18 +21,6 @@ | |||
// TODO(amirh): once GL support is in evaluate if we can merge this with FlutterView. | |||
@implementation FlutterOverlayView | |||
|
|||
- (instancetype)initWithFrame:(CGRect)frame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-objc-avoid-throwing-exception
I ignored this check in .clang-tidy
since there are a few more tricky ones to fix. However, these throwing implementations should be removed, they are already marked as NS_UNAVAILABLE
in the header.
- (instancetype)initWithFrame:(CGRect)frame NS_UNAVAILABLE; | |
- (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NS_UNAVAILABLE won't catch all usage of these selectors. How about refactoring this to NSAssert calls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly. Xib's for example will execute initWithCoder:
. It uses the objc runtime to execute the selector so NS_UNAVAILABLE won't effect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, for UIView
subclasses and similar, good catch.
@@ -39,12 +39,6 @@ @implementation FlutterPlatformPlugin { | |||
fml::WeakPtr<FlutterEngine> _engine; | |||
} | |||
|
|||
- (instancetype)init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- (instancetype)init NS_UNAVAILABLE; | |
+ (instancetype)new NS_UNAVAILABLE; |
@@ -20,13 +20,6 @@ @implementation FlutterRestorationPlugin { | |||
BOOL _restorationEnabled; | |||
} | |||
|
|||
- (instancetype)init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
engine/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h
Lines 13 to 14 in d5d7526
- (instancetype)init NS_UNAVAILABLE; | |
+ (instancetype)new NS_UNAVAILABLE; |
@@ -21,24 +21,6 @@ @implementation FlutterView { | |||
id<FlutterViewEngineDelegate> _delegate; | |||
} | |||
|
|||
- (instancetype)init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
engine/shell/platform/darwin/ios/framework/Source/FlutterView.h
Lines 38 to 41 in d5d7526
- (instancetype)init NS_UNAVAILABLE; | |
+ (instancetype)new NS_UNAVAILABLE; | |
- (instancetype)initWithFrame:(CGRect)frame NS_UNAVAILABLE; | |
- (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE; |
@@ -15,12 +15,6 @@ @implementation FlutterTextureRegistrar { | |||
NSMutableDictionary<NSNumber*, id<FlutterMacOSExternalTexture>>* _textures; | |||
} | |||
|
|||
- (instancetype)init { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- (nullable instancetype)init NS_UNAVAILABLE; |
@@ -27,4 +27,4 @@ id CreateMockViewController(NSString* pasteboardString) { | |||
} | |||
} | |||
|
|||
} | |||
} // namespace flutter::testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-readability-namespace-comments
wasn't turned on, but I spotted it, may as well fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo throws should be changed to NSAsserts.
@@ -21,18 +21,6 @@ | |||
// TODO(amirh): once GL support is in evaluate if we can merge this with FlutterView. | |||
@implementation FlutterOverlayView | |||
|
|||
- (instancetype)initWithFrame:(CGRect)frame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NS_UNAVAILABLE won't catch all usage of these selectors. How about refactoring this to NSAssert calls?
b9e12dc
to
b986931
Compare
b986931
to
3c168bb
Compare
Run
clang-tidy
onhost_debug
andios_debug
forgoogle-objc-*
analyzer checks.Part of flutter/flutter#93279 and flutter/flutter#91857
Pre-launch Checklist
writing and running engine tests.
///
).