Improve Mac compatibility for hsMessageBox#1766
Conversation
|
I wonder if the autoreleasepool is needed at all, if the code is all linear/blocking? I think you could just do a regular |
An autorelease pool would be needed because Cocoa may internally autorelease things. Whenever one interacts with Cocoa there should be an active autorelease pool unless the documentation supports it being unnecessary. But what I don't know is if this code inherits the auto release pool from the main thread. But I'm ok with adding one just in case it doesn't. |
| NSModalResponse response = [alert runModal]; | ||
| result = static_cast<hsMessageBoxResult>(response); | ||
| } else { | ||
| HSMBPresenter* presenter = [[HSMBPresenter alloc] init]; |
There was a problem hiding this comment.
Nit: Third party Cocoa prefixes should be three letters according to the language guidelines.
| HSMBPresenter* presenter = [[HSMBPresenter alloc] init]; | ||
|
|
||
| #if !__has_feature(objc_arc) | ||
| [presenter autorelease]; |
There was a problem hiding this comment.
I need to pull down the commit and take a look - so don't change anything yet... But in theory ARC would need an autorelease pool too. ARC doesn't automatically create autorelease pools. But unfortunately autorelease pool creation is different under ARC.
There was a problem hiding this comment.
In its current state, this PR uses an @autoreleasepool block in ARC mode and a traditional NSAutoreleasePool in non-ARC mode. That should be enough, right?
|
Now I'm wondering, is there any time this file would be compiled with ARC and need to use |
|
We should check on that. It may not matter after the changes - but this code looks like it assumed ARC so it may have been leaky if ARC wasn't on. |
dpogue
force-pushed
the
mac-messagebox
branch
from
03497f7 to
05911d4
Compare
|
Marked as draft because I rebased it on top of #1776 and am waiting for review on that |
|
I've rebased this and gotten rid of the autorelease usage... but I am definitely not certain that I've got the retain/release stuff right |
|
My memory of this PR is out of date. But I'll wait for the memory issues to be addressed before looking at this again. |
Co-Authored-By: dgelessus <dgelessus@users.noreply.github.com>
3 participants
@colincornaby Curious for your thoughts here...
The big change is moving away from blocks (which are not GCC-compatible and don't work on Mac OS X <10.6) to
performSelectorOnMainThread:The part I am least certain about is (as always) the autorelease stuff. Ugh.