Skip to content
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

Xcode 5 static analyzer fixes #1627

Merged
merged 1 commit into from Sep 30, 2013
Merged

Xcode 5 static analyzer fixes #1627

merged 1 commit into from Sep 30, 2013

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 30, 2013

I ran the static analyzer from Xcode 5. Lots of things in the ND classes. The things here are small, mainly typos.

A look over the __bridge_transfer by @craigotis would be good :)

P.S. yes - the analyzer did bring up the cleanObjectDictionary method ;-)

@craigotis
Copy link
Contributor

@craigotis craigotis commented Sep 30, 2013

That is correct - and a memory leak that I missed. Just using __bridge converts from a C-pointer (or CFTypeRef) to an Objective-C style id. However, the ownership was not transferred to ARC, it just said, "This CFTypeRef is now an Objective-C object. Trust me, I've got this."

But, because ownership was never transferred to ARC, the NSString is never cleaned up. Adding the __bridge_transfer gives ownership of the object to ARC, which is good, because we're creating the object that we're returning. (As Create is in the function name.)

Looks good to me.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 30, 2013

Thanks for the clarification Craig. I thought it was better :)
So ARC knows that because the method doesn't contain init in the name, it should autorelease the object (now that it owns it)?

I guess you could merge if you like :)

@craigotis
Copy link
Contributor

@craigotis craigotis commented Sep 30, 2013

It's not really ARC reading the name (maybe it does when suggesting quick fixes or static analysis, I'm not sure), but more about conventions set forth here:

https://developer.apple.com/library/ios/DOCUMENTATION/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-103029

Check out the section, "The Create Rule"

So any time you use CF...Create... or CF...Copy..., you own whatever was returned, and are responsible for releasing it, as you are the reason it now has an unmatched +1 retain count.

When you just use __bridge, it means that no ownership transfer occurs. Whatever kind of object you cast the pointer to (though it's usually a toll-free bridged type like NSString), you'd have to call release on that object. This is because its creation occurred outside what ARC is capable of managing and understanding. (The "C realm", instead of the "Objective-C realm".)

So if you wanted to manage that memory manually, since we're using ARC and can't call release, you'd have to cast it back to a CFTypeRef and call CFRelease() someday.

But, we don't want to do that! We're creating this string one-off and giving it away as a return value. So by using __bridge_transfer, we're dropping the object at ARC's doorstep and saying, "You know what? You take this one. We don't want to worry about it."

This essentially consumes the +1 retain count that the object had. ARC now owns the object, and is responsible for releasing it when it normally would release any other ARC-managed object. (At the end of the "enclosing full-expression.")

This is a good SO answer, that could not be more relevant:

http://stackoverflow.com/a/10476359/88111

craigotis added a commit that referenced this issue Sep 30, 2013
@craigotis craigotis merged commit df8ea0b into master Sep 30, 2013
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 30, 2013

Yep, I understand the Create rule ;-)

It just seems clever to me that ARC will know that the return object from that method is an autoreleased object as opposed to a retained object (or is it).

So when we call somewhere else in our code

NSString *myString = [NSString uniqueString];

ARC knows that it doesn't need to release this object (hopefully) since -[NSString uniqueString] returns an autoreleased object (hopefully!)

On 30 Sep 2013, at 23:17, Craig Otis notifications@github.com wrote:

It's not really ARC reading the name (maybe it does when suggesting quick fixes or static analysis, I'm not sure), but more about conventions set forth here:

https://developer.apple.com/library/ios/DOCUMENTATION/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-103029

Check out the section, "The Create Rule"

So any time you use CF...Create... or CF...Copy..., you own whatever was returned, and are responsible for releasing it, as you are the reason it now has an unmatched +1 retain count.

When you just use __bridge, it means that no ownership transfer occurs. Whatever kind of object you cast the pointer to (though it's usually a toll-free bridged type like NSString), you'd have to call release on that object. This is because its creation occurred outside what ARC is capable of managing and understanding. (The "C realm", instead of the "Objective-C realm".)

So if you wanted to manage that memory manually, since we're using ARC and can't call release, you'd have to cast it back to a CFTypeRef and call CFRelease() someday.

But, we don't want to do that! We're creating this string one-off and giving it away as a return value. So by using __bridge_transfer, we're dropping the object at ARC's doorstep and saying, "You know what? You take this one. We don't want to worry about it."

This essentially consumes the +1 retain count that the object had. ARC now owns the object, and is responsible for releasing it when it normally would release any other ARC-managed object. (At the end of the "enclosing full-expression.")

This is a good SO answer, that could not be more relevant:

http://stackoverflow.com/a/10476359/88111


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Sep 30, 2013

Yea, I wouldn't so much think of it as an 'autoreleased' object, just as an object that ARC now knows, when compiling, to insert the appropriate release methods for.

When we call:

NSString *myString = [NSString uniqueString];

Because that method uses __bridge_transfer before it returns the NSString object, ARC actually knows that it does need to release that object.

However, the nice thing about ARC/Clang is that it examines the places where the myString object are referenced, and rather than having to examine retain counts at runtime (as a garbage collector might), it can analyze the actual lifecycle of the object and insert [myString release] at the appropriate places.

@pjrobertson pjrobertson deleted the staticAnalyzer branch Sep 30, 2013
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 30, 2013

Aaaah I see. ARC's THAT clever. It looks at the whole scope/lifetime of a variable to see what to do with it.
… but my next question:

what if ARC only has the header file (your average framework) for

+[NSString uniqueID]

so it doesn't know that it's a __bridge_transfer object?
I appreciate the explanations :)

On 30 Sep 2013, at 23:39, Craig Otis notifications@github.com wrote:

Yea, I wouldn't so much think of it as an 'autoreleased' object, just as an object that ARC now knows, when compiling, to insert the appropriate release methods for.

When we call:

NSString *myString = [NSString uniqueString];

Because that method uses __bridge_transfer before it returns the NSString object, ARC actually knows that it does need to release that object.

However, the nice thing about ARC/Clang is that it examines the places where the myString object are referenced, and rather than having to examine retain counts at runtime (as a garbage collector might), it can analyze the actual lifecycle of the object and insert [myString release] at the appropriate places.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Sep 30, 2013

If it only has the header file, I'm assuming the implementation file has already been compiled. (Let's say it's a static library.)

In this case, it's safe to assume that the method in the pre-compiled binary already includes an autorelease call, something like:

    ...
    return [uuidStr autorelease];
}

ARC actually uses autorelease pools (and autorelease calls) behind the scenes - it just doesn't want you to use them. So this returned object will be released during the next drain of the currently-scoped pool. If you assign the return value of this method to a strong property on an object, then as you'd expect, ARC will add the retain and release calls for the property, and once the final release has been called, the autorelease pool will either deliver the killing blow, or if the pool has already been drained, the final release for the property will send the object to the grave.

I'll look into this to be sure, though. And it goes without saying that in a situation where you have the .m files, you can just use the -fno-objc-arc flag.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented Sep 30, 2013

In this case, it's safe to assume that the method in the pre-compiled binary already includes an autorelease call, something like:

So in the case of QSPlugins that use the +[NSString uniqueString] method (examples include the Web Search plugin, Displays plugin and @skurfer's unreleased CloudApp plugin), ARC (and our own manual memory management brains - since it's a convenience method) is assuming that the object is autoreleased - surely we should follow this convention?

On 1 Oct 2013, at 00:24, Craig Otis notifications@github.com wrote:

If it only has the header file, I'm assuming the implementation file has already been compiled. (Let's say it's a static library.)

In this case, it's safe to assume that the method in the pre-compiled binary already includes an autorelease call, something like:

...
return [uuidStr autorelease];

}
ARC actually uses autorelease pools (and autorelease calls) behind the scenes - it just doesn't want you to use them. So this returned object will be released during the next drain of the currently-scoped pool. If you assign the return value of this method to a strong property on an object, then as you'd expect, ARC will add the retain and release calls for the property, and once the final release has been called, the autorelease pool will either deliver the killing blow, or if the pool has already been drained, the final release in for the property will send the object to the grave.

I'll look into this to be sure, though. And it goes without saying that in a situation where you have the .m files, you can just use the -fno-objc-arc flag.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Sep 30, 2013

Yes, but this has always been the case.

If you're calling a method that returns an object, and does not start with alloc, new, copy, or mutableCopy, then you should assume that the returned object will disappear if you don't explicitly gain ownership with a retain call.

Where is this convention not being followed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants