Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
MNT: Change objective C code to Automatic Reference Counting (ARC)#23060
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The backend seems to continue to function after this. However, I'm not sure how you would like it tested. |
I'm not sure it is "testable"... Here is the traceback I was using to make it compatible if I enable ARC on main currently. This was in an effort to trace down the memory leaks. Here is the ARC recommendation:https://microsoft.github.io/objc-guide/MethodsAndImplementations/AutomaticReferenceCounting.html#convention expand for traceback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know anything about this, other than that this PR works on my machine and it doesn't appear to do anything malicious. Is there anyone else who ha sufficient macOS experience to vouch for this? Also is ARC sufficiently back compatible?
It came in MacOSX 10.7, so it has been around for quite a while now. Note that I did put a check at the top of the file as well so this will throw an error during compile if ARC is not enabled. |
marking as merge w/ one review unless another Mac user wants to review I will merge in a week or so... |
| @@ -1064,36 +1060,6 @@ + (WindowServerConnectionManager *)sharedManager | |||
| return sharedWindowServerConnectionManager; | |||
| } | |||
| + (id)allocWithZone:(NSZone *)zone | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://developer.apple.com/documentation/objectivec/nsobject/1571945-allocwithzone
Do not override allocWithZone: to include any initialization code. Instead, class-specific versions of init... methods.
OK, good thing we are deleting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it is also not clear we used this as the actual call site just above uses[[super allocWithZone:Null] init] ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This all came in via7bacdc5 with the commit message
This patch allows interrupts to be delivered once Python is fixed.
This updates the objective C macosx backend to use automatic referencecounting instead of relying on manual tracking and releasing of the memory.
I took the liberty of rebasing and removing the line that was breaking the wheel build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please merge this@greglucas if you are happy with the commit I added.
Yes, that commit looks good, it must have come in since the last rebase. Thanks for fixing it! |
PR Summary
This updates the objective C macosx backend to use automatic reference counting instead of relying on manual tracking and releasing of the memory. It seems like this is recommended now, but I am definitely not an expert in this.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).