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

Fix objc2 warnings #34

Draft
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Jul 20, 2022

Builds upon #30.

Fix warnings introduced by objc2 deprecating certain functionality.

/// Bool mapping types differ between ARM and x64. There's a number of places that we need to check
/// against BOOL results throughout the framework, and this just simplifies some mismatches.
#[inline(always)]
pub fn to_bool(result: BOOL) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me stupidly happy to see deleted.

@ryanmcgrath
Copy link
Owner

This one also looks fine to me, albeit needs a touch of rebase it seems.

@madsmtm madsmtm force-pushed the objc2-warnings branch 2 times, most recently from 0942f27 to d281529 Compare August 29, 2022 03:01
@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 29, 2022

I've incorporated a few fixes from a newer version of objc2 into this, namely the ability to just use bool in place of BOOL, and to elide a lot of .unwrap()'s after msg_send_id!.

pub(crate) fn register_window_controller_class<T: WindowDelegate>() -> *const Class {
static mut DELEGATE_CLASS: *const Class = 0 as *const Class;
pub(crate) fn register_window_controller_class<T: WindowDelegate>() -> &'static Class {
static mut DELEGATE_CLASS: Option<&'static Class> = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be an Option<&'static Class> because it was initialized as 0 as *const Class before? I think the only part that feels odd to me is the unsafe { DELEGATE_CLASS.unwrap() }.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static is created before the program begins, so it has to be initialized to some value, in this case None. When the function is called the first time, the class is created and the static is set to the new value.
The unsafety in unsafe { DELEGATE_CLASS.unwrap() } comes from the fact that DELEGATE_CLASS is a mutable static, and hence can only be accessed behind unsafe. Data races are prevented by Once.

In essence, what we do here with DELEGATE_CLASS and Once is similar to once_cell::sync::Lazy and the nightly std::cell::LazyCell.

@ryanmcgrath
Copy link
Owner

Where's this one sit with regards to #30? Should I just ignore it at this point?

@madsmtm
Copy link
Contributor Author

madsmtm commented Aug 1, 2023

No, I'll pick it up again after #30

@ryanmcgrath
Copy link
Owner

Kk sounds good!

@0123456789-jpg
Copy link

Is this still being worked on? It seems like the usage of beta objc2 makes it nearly impossible to use objc2-* crates or even icrate without doubling objc2 depdency. I suppose this PR is the most relevant to bumping and porting the whole thing to a newer objc2 version.

@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 16, 2024

No, I haven't worked on this crate in a long while, unfortunately. I still think there is value in what it is providing (a more rustic interface to Apple APIs), but my own focus has been on improving objc2 itself (there's still a lot to do to just get "an interface to Apple APIs" before we can really focus on the "rustic" part).

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.

4 participants