Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764

jmagman · 2021-11-16T04:35:29Z

Run clang-tidy on host_debug and ios_debug for google-objc-* analyzer checks.

Part of flutter/flutter#93279 and flutter/flutter#91857

$ ../buildtools/mac-x64/clang/bin/clang-tidy --dump-config
Checks:          'clang-diagnostic-*,clang-analyzer-*,google-*,-google-objc-global-variable-declaration,-google-objc-avoid-throwing-exception'
WarningsAsErrors: 'clang-analyzer-osx.*,google-objc-*'

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

jmagman · 2021-11-16T04:36:04Z

shell/platform/darwin/common/framework/Source/FlutterChannels.mm

@@ -173,7 +173,7 @@ - (NSUInteger)hash {
 }
 @end

-NSObject const* FlutterMethodNotImplemented = [NSObject new];
+NSObject const* FlutterMethodNotImplemented = [[NSObject alloc] init];


google-objc-avoid-nsobject-new

jmagman · 2021-11-16T04:36:15Z

shell/platform/darwin/common/framework/Source/flutter_standard_codec_unittest.mm

@@ -6,7 +6,7 @@

 #include "gtest/gtest.h"

-void checkEncodeDecode(id value, NSData* expectedEncoding) {
+static void CheckEncodeDecode(id value, NSData* expectedEncoding) {


google-objc-function-naming

jmagman · 2021-11-16T04:38:14Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

@@ -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 {


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.

engine/shell/platform/darwin/ios/framework/Source/FlutterOverlayView.h

Lines 33 to 34 in d5d7526

- (instancetype)initWithFrame:(CGRect)frame NS_UNAVAILABLE;

- (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE;

NS_UNAVAILABLE won't catch all usage of these selectors. How about refactoring this to NSAssert calls?

What situations does it not cover? Are you worried about the selector being send to it?

@interface Foo : NSObject - (instancetype)init NS_UNAVAILABLE; - (instancetype)initWithStuff NS_DESIGNATED_INITIALIZER; - (void)doStuff; @end

@interface Bar : Foo @end

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.

You're right, for UIView subclasses and similar, good catch.

jmagman · 2021-11-16T04:39:06Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.mm

@@ -39,12 +39,6 @@ @implementation FlutterPlatformPlugin {
  fml::WeakPtr<FlutterEngine> _engine;
 }

- (instancetype)init {


engine/shell/platform/darwin/ios/framework/Source/FlutterPlatformPlugin.h

Lines 13 to 14 in d5d7526

- (instancetype)init NS_UNAVAILABLE;

+ (instancetype)new NS_UNAVAILABLE;

jmagman · 2021-11-16T04:39:35Z

shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.mm

@@ -20,13 +20,6 @@ @implementation FlutterRestorationPlugin {
  BOOL _restorationEnabled;
 }

- (instancetype)init {


engine/shell/platform/darwin/ios/framework/Source/FlutterRestorationPlugin.h

Lines 13 to 14 in d5d7526

- (instancetype)init NS_UNAVAILABLE;

+ (instancetype)new NS_UNAVAILABLE;

jmagman · 2021-11-16T04:40:14Z

shell/platform/darwin/ios/framework/Source/FlutterView.mm

@@ -21,24 +21,6 @@ @implementation FlutterView {
  id<FlutterViewEngineDelegate> _delegate;
 }

- (instancetype)init {


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;

jmagman · 2021-11-16T04:40:46Z

shell/platform/darwin/macos/framework/Source/FlutterTextureRegistrar.mm

@@ -15,12 +15,6 @@ @implementation FlutterTextureRegistrar {
  NSMutableDictionary<NSNumber*, id<FlutterMacOSExternalTexture>>* _textures;
 }

- (instancetype)init {


engine/shell/platform/darwin/macos/framework/Source/FlutterTextureRegistrar.h

Line 30 in d5d7526

- (nullable instancetype)init NS_UNAVAILABLE;

jmagman · 2021-11-16T04:42:31Z

shell/platform/darwin/macos/framework/Source/FlutterViewControllerTestUtils.mm

@@ -27,4 +27,4 @@ id CreateMockViewController(NSString* pasteboardString) {
  }
 }

-}
+}  // namespace flutter::testing


google-readability-namespace-comments wasn't turned on, but I spotted it, may as well fix it.

gaaclarke

LGTM modulo throws should be changed to NSAsserts.

gaaclarke · 2021-11-16T21:07:52Z

shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm

@@ -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 {


NS_UNAVAILABLE won't catch all usage of these selectors. How about refactoring this to NSAssert calls?

…lutter/engine#29764)

jmagman self-assigned this Nov 16, 2021

flutter-dashboard bot added platform-ios platform-macos labels Nov 16, 2021

google-cla bot added the cla: yes label Nov 16, 2021

jmagman commented Nov 16, 2021

View reviewed changes

jmagman requested review from zanderso and gaaclarke November 16, 2021 19:44

gaaclarke approved these changes Nov 16, 2021

View reviewed changes

jmagman force-pushed the clang-google-objc branch from b9e12dc to b986931 Compare November 17, 2021 02:17

jmagman added 3 commits November 17, 2021 11:01

Fix google-objc-*

219db43

NSAssert

b3cb4f7

Fix nil return

3c168bb

jmagman force-pushed the clang-google-objc branch from b986931 to 3c168bb Compare November 17, 2021 19:02

jmagman added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 17, 2021

fluttergithubbot merged commit 598ac02 into flutter:main Nov 18, 2021

jmagman deleted the clang-google-objc branch November 18, 2021 00:50

engine-flutter-autoroll mentioned this pull request Nov 18, 2021

Roll Engine from c30375b830ef to a3ef1d5351f4 (5 revisions) flutter/flutter#93839

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 18, 2021

598ac02 Fix "google-objc-*" clang analyzer warning in macOS and iOS (f…

ccb74da

…lutter/engine#29764)

jmagman mentioned this pull request Nov 23, 2021

Run clang tidy on more targets flutter/flutter#61661

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764

Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764

	- (instancetype)initWithFrame:(CGRect)frame NS_UNAVAILABLE;
	- (instancetype)initWithCoder:(NSCoder*)aDecoder NS_UNAVAILABLE;

	- (instancetype)init NS_UNAVAILABLE;
	+ (instancetype)new NS_UNAVAILABLE;

Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764

Fix "google-objc-*" clang analyzer warning in macOS and iOS #29764

Conversation

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment