Update Reachability.m #123
Conversation
The changes fix the leak reported by the Analyzer in XCode 7. Issue tonymillion#114
| id reachability = [[self alloc] initWithReachabilityRef:ref]; | ||
|
|
||
| id reachability = [[self alloc] initWithReachabilityRef:CFBridgingRetain((__bridge id)ref)]; | ||
| CFRelease(ref); |
There was a problem hiding this comment.
Hey there, I saw your fix and wanted to leave a little feedback. I've been monitoring this issue closely I think you're the fourth person now I've interacted on a PR with :)
At line 98 retain count of the ref will be 1, due to the create.
We don't know immediately what the init does, so lets assume it doesn't retain it.
Now here on 102, you release ref. Its retain count is now 0. You will have created a zombie
(Final note) The init doesn't retain it, so all the above assumptions are true. Try and run the app with your changes, the reachability class will hit a zombie. See my solution #118 for the full fix. You are correct in that you whenever you create a foundational item you should release it. However what you, and a couple others, ran into was the assumption that the init takes ownership. Which it doesn't at the moment. If you update the init to take ownership then your fix will be complete. :)
There was a problem hiding this comment.
Ah, I see what you did. I missed it and I assumed you did what others did before.
You can disregard my above comments.
However, I'd like to leave 2 cents if you don't mind :)
Not sure about everyone else but for me your fix is a little hard to read. I totally glanced over your retain statement. Since the ultimate (final) release is in the dealloc, I think many people would expect the retain to happen in the init. Instead of all the helpers that call the init. And while there may not be additional helpers being added any time soon, if someone does, they'll have to be sure to make the retain in their new helper. Since the current helpers are class methods, it does feel a little odd its doing memory management on an instance item.
Again, just my two cents & it may be a little nit-picky, but feels a little anti-pattern.
There was a problem hiding this comment.
As much as the analyzer doesn't report possible problems, I don't really care what solution is bes among those two. They both look fine to me.
However don't blame others if you don't read code carefully... ;-)
The changes fix the leak reported by the Analyzer in XCode 7.
Issue #114