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

Initial transition to objc2 #2452

Merged
merged 4 commits into from Sep 2, 2022
Merged

Initial transition to objc2 #2452

merged 4 commits into from Sep 2, 2022

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Aug 31, 2022

Move to objc2, my fork of objc which adds a lot of things, in particular macros for proper memory management, and a lot of fixed soundness issues.

Split out from of #2427.

Concrete improvements in objc2 which can be seen in this PR:

Checklist:

  • Tested on all platforms changed
    • macOS 10.14
    • iOS simulator (can't get log output here though, so a bit harder)
    • iOS 9.3.6 (iPad mini 1st gen)
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Cargo.toml Outdated
Comment on lines 68 to 72
# Branch: objc2
cocoa = { git = "https://github.com/madsmtm/core-foundation-rs.git", rev = "8e6c4854de2408ed1e3e614dbe6f9f2762bde7c3" }
core-foundation = { git = "https://github.com/madsmtm/core-foundation-rs.git", rev = "8e6c4854de2408ed1e3e614dbe6f9f2762bde7c3" }
core-graphics = { git = "https://github.com/madsmtm/core-foundation-rs.git", rev = "8e6c4854de2408ed1e3e614dbe6f9f2762bde7c3" }
Copy link
Member Author

@madsmtm madsmtm Aug 31, 2022

Choose a reason for hiding this comment

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

This is only in the interim; once #2427 is fully merged, the need for this will be gone

CHANGELOG.md Outdated
@@ -8,6 +8,8 @@ And please only add new entries to the top of this list, right below the `# Unre

# Unreleased

- **Breaking:** Bump MSRV from `1.57` to `1.60`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that objc2 doesn't have a MSRV policy yet, see madsmtm/objc2#203.

But since we use a pinned dependency, this shouldn't be a problem (yet).

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I don't have that much to say, given that I have no clue about the objc internals. The changes look good, some minor notes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@@ -175,8 +167,8 @@ impl fmt::Debug for MonitorHandle {

impl MonitorHandle {
pub fn retained_new(uiscreen: id) -> MonitorHandle {
assert_main_thread!("`MonitorHandle` can only be cloned on the main thread on iOS");
Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with that PR, but could libdispatch used to drop from the main thread? So we won't have runtime panics like that? Though I think the issue that you can't use that from the other thread as well?

It just all looks wrong to me. We say that winit window is Send and you can get MonitorHandle from the Window(owning one iirc), but when you drop it you'll panic?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is so much thread-unsafety in our macOS (and probably also iOS, but less well-versed in that) implementation, really, you have no idea!

Anyway, to answer your question: Yes, we could and should use libdispatch to do things on the main thread, both here and in a lot of other places. I'm in the process of exploring solutions, have focused on objc2 stuff first though.

@madsmtm madsmtm mentioned this pull request Sep 1, 2022
2 tasks
@madsmtm madsmtm force-pushed the objc2-initial branch 2 times, most recently from 58f3022 to 34fb9ad Compare September 2, 2022 09:05
@madsmtm madsmtm merged commit 112965b into master Sep 2, 2022
@madsmtm madsmtm deleted the objc2-initial branch September 2, 2022 13:48
@madsmtm madsmtm added this to the Version 0.28 milestone Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants