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

Annotate all Cocoa properties as nonatomic. #325

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Conversation

jverkoey
Copy link
Contributor

@jverkoey jverkoey commented Jan 13, 2021

As best I can tell, none of the properties in the Cocoa directory truly rely on atomicity, so explicitly indicating this accomplishes three things:

  1. Aligns with typical Objective-C convention of marking single-threaded properties nonatomic unless atomicity is explicitly required.
  2. Removes any unnecessary overhead that may have been introduced by the Objective-C runtime in order to synthesize atomicity.
  3. Refactoring of Document.h/m can follow typical conventions more closely.

Note, though, that it appears that the emulator does run on a background thread which in turn interacts with Cocoa views. This can lead to undefined behavior because Cocoa views aren't threadsafe. This can actually be seen in the logs with a warning for Main Thread Checker: UI API called on a background thread: -[NSWindow isVisible]. Ensuring that main-thread elements are only accessed via the main thread may be a reasonable follow-up.

Pre-work for #324.

@LIJI32 LIJI32 merged commit 931045f into LIJI32:master Jan 15, 2021
@LIJI32
Copy link
Owner

LIJI32 commented Jan 15, 2021

Good catch, thanks!

As for main thread safety – I went through all these warnings at one point, fixing all of these with the exception of the one you just noted. The reason is that adding a a dispatch call in that use case created severe performance drops, so I was willing to take this thread-safety risk considering it's a bool property that doesn't change very frequently.

If you have a solution for this without using a dispatch, it'll be welcome; I hate this warning as much as you do :P

@jverkoey
Copy link
Contributor Author

Filed #331 to track looking into the warning :)

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.

2 participants