Fix miscellaneous static analyzer warnings #7670

1ec5 · 2017-01-11T06:07:25Z

This PR fixes the following static analyzer warnings, which were introduced in #6709, #7377, #6559, and #7300:

/path/to/mapbox-gl-native/platform/darwin/src/MGLNetworkConfiguration.m:16:9: Null is returned from a method that is expected to return a non-null value
/path/to/mapbox-gl-native/platform/darwin/src/MGLRasterSource.mm:63:23: Function call argument is an uninitialized value (within a call to 'make_unique')
/path/to/mapbox-gl-native/platform/darwin/src/MGLShape.mm:98:10: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage
/path/to/mapbox-gl-native/platform/ios/src/MGLAnnotationImage.m:60:54: Null passed to a callee that requires a non-null 1st parameter
/path/to/mapbox-gl-native/platform/ios/src/MGLAnnotationImage.m:65:10: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage
/path/to/mapbox-gl-native/platform/macos/src/MGLAnnotationImage.m:61:10: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage
/path/to/mapbox-gl-native/platform/macos/src/NSImage+MGLAdditions.mm:13:5: Returning 'self' while it is not set to the result of '[(super or self) init...]'

Working towards #7668.

/cc @frederoni @incanus

Fixed static analyzer warnings in MGLNetworkConfiguration, MGLRasterSource, and MGLShape.

Fixed static analyzer warnings in MGLAnnotationImage.

Fixed static analyzer warnings in MGLAnnotationImage and NSImage(MGLAdditions).

mention-bot · 2017-01-11T06:07:26Z

@1ec5, thanks for your PR! By analyzing this pull request, we identified @incanus and @boundsj to be potential reviewers.

1ec5 · 2017-01-11T06:08:50Z

platform/darwin/src/MGLNetworkConfiguration.m

@@ -12,9 +11,6 @@ + (void)load {
 }

 + (instancetype)sharedManager {
-    if (NSProcessInfo.processInfo.mgl_isInterfaceBuilderDesignablesAgent) {


+[MGLAccountManager sharedManager] bails in IB because MGLAccountManager spins up lots of other machinery, including MGLMapboxEvents. MGLNetworkConfiguration is intentionally isolated from anything else, so initializing the singleton within IB is no big deal.

1ec5 · 2017-01-11T06:10:57Z

platform/ios/src/MGLAnnotationImage.m

@@ -57,15 +57,11 @@ - (BOOL)isEqual:(id)other {

    return ((!_reuseIdentifier && !otherAnnotationImage.reuseIdentifier) || [_reuseIdentifier isEqualToString:otherAnnotationImage.reuseIdentifier])
    && _enabled == otherAnnotationImage.enabled
-    && ((!_image && !otherAnnotationImage.image) || [UIImagePNGRepresentation(_image) isEqualToData:UIImagePNGRepresentation(otherAnnotationImage.image)]);


@frederoni, any reason why comparing two UIImages for object equality isn’t good enough? (There’s a similar comparison of NSImages’ TIFF representations on the macOS side.)

I see that testAnnotationImage() is failing as a result of my change. ~~It’s true that drawing the image to a new context could make the two images unequal, but there are a number of reasons that would be the case anyways (such as different color profiles).~~ In general, I don’t think a class’s -isEqual: implementation should be more thorough than the -isEqual: implementations of its ivars.

Edit: I misread the test. Looks like it’s doing something pretty ordinary.

1ec5 · 2017-01-11T06:11:06Z

platform/darwin/src/MGLRasterSource.mm

@@ -51,7 +51,7 @@ - (instancetype)initWithIdentifier:(NSString *)identifier tileURLTemplates:(NS_A
    if (self = [super initWithIdentifier:identifier tileURLTemplates:tileURLTemplates options:options]) {
        mbgl::Tileset tileSet = MGLTileSetFromTileURLTemplates(tileURLTemplates, options);

-        uint16_t tileSize;


I had somehow forgotten that primitive-typed stack variables are uninitialized in Objective-C just as they are in C. ARC nils out only object pointers on the stack.

kkaefer · 2017-01-11T09:14:40Z

platform/darwin/src/MGLShape.mm

-    NSUInteger hash;
-    hash += _title.hash;
-    hash += _subtitle.hash;
+    NSUInteger hash = _title.hash + _subtitle.hash;


Is this a safe way to compute the hash?

A XOR over the values could improve it but I'm ok with it because this is an abstract class and can't be used directly. All concrete subclasses include the hash of the GeoJSON dictionary.

This is a fairly conventional implementation, as far as I know. The only hard requirement for a -hash implementation is that two equal objects (as in -isEqual:) must have the same hash. But feel free to improve on the various -hash implementations in this SDK in a separate PR.

1ec5 added 3 commits January 10, 2017 21:58

[ios, macos] Fixed static analyzer warnings

4adb075

Fixed static analyzer warnings in MGLNetworkConfiguration, MGLRasterSource, and MGLShape.

[ios] Fixed static analyzer warnings

37ef97a

Fixed static analyzer warnings in MGLAnnotationImage.

[macos] Fixed static analyzer warnings

cce1231

Fixed static analyzer warnings in MGLAnnotationImage and NSImage(MGLAdditions).

1ec5 added annotations Annotations on iOS and macOS or markers on Android build iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Jan 11, 2017

1ec5 added this to the ios-v3.4.0 milestone Jan 11, 2017

1ec5 self-assigned this Jan 11, 2017

1ec5 requested a review from frederoni January 11, 2017 06:07

1ec5 commented Jan 11, 2017

View reviewed changes

1ec5 mentioned this pull request Jan 11, 2017

Static analyzer warnings, mostly in runtime styling classes #7668

Open

[ios, macos] Two pointer-equal images are object-equal by definition

d7e63d0

kkaefer reviewed Jan 11, 2017

View reviewed changes

frederoni approved these changes Jan 11, 2017

View reviewed changes

1ec5 merged commit 581fb38 into release-ios-v3.4.0 Jan 11, 2017

1ec5 deleted the 1ec5-analyze-7668 branch January 11, 2017 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix miscellaneous static analyzer warnings #7670

Fix miscellaneous static analyzer warnings #7670

1ec5 commented Jan 11, 2017 •

edited

Loading

mention-bot commented Jan 11, 2017

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017

kkaefer Jan 11, 2017

frederoni Jan 11, 2017

1ec5 Jan 11, 2017 •

edited

Loading

Fix miscellaneous static analyzer warnings #7670

Fix miscellaneous static analyzer warnings #7670

Conversation

1ec5 commented Jan 11, 2017 • edited Loading

mention-bot commented Jan 11, 2017

1ec5 Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 Jan 11, 2017

Choose a reason for hiding this comment

kkaefer Jan 11, 2017

Choose a reason for hiding this comment

frederoni Jan 11, 2017

Choose a reason for hiding this comment

1ec5 Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

1ec5 commented Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading

1ec5 Jan 11, 2017 •

edited

Loading