-
Notifications
You must be signed in to change notification settings - Fork 224
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
Replace objc
with objc2
#628
base: main
Are you sure you want to change the base?
Conversation
Using new objc-encode crate
(That the semver checks are failing is expected, this is a breaking change). |
@madsmtm Do you have a read on how this crate differs from the relevant higher-level crates in the objc2 family? (are there equivalent crate(s)). Would it make sense to look at more heavily adopting some of those crates? Or even outright replacing (some of?) the crates in this repo with ones from https://github.com/madsmtm/objc2 ? |
There's some fairly old discussion of a few lower-level details in #513, but yeah, nowadays we have
|
A couple of followup questions:
|
They do have a compile-time cost, but it's quite low by now if you only enable the features you need, see the docs. Wrt. code-size, as long as you use LTO, there shouldn't be a difference. I'm considering adding
CoreText is in the category of a |
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 think it is worth extracting at least 2 parts of this out (the changes to core-text
and core-graphics-types
) and considering them separately from the changes to cocoa
and cocoa-foundation
.
@@ -8,7 +8,8 @@ | |||
// except according to those terms. | |||
|
|||
extern crate core_foundation; | |||
extern crate libc; | |||
#[cfg(feature = "objc2")] |
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 think there's value in having a standalone PR that adds the Encode
traits for objc2
to core-graphics-types
via a feature. Would that help for some of what you maintain in the objc2
family of crates?
It is separate from converting the cocoa
and cocoa-foundation
crates, right?
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.
Hmm, I think it would help, though I'll have to mull it over; it kinda depends on how long it will take me to get to doing madsmtm/objc2#556 (in which case all the types in this repo will just be autogenerated), and I suspect a lot of people would be annoyed at the extra dependency.
Gotta admit, I'm not really motivated to work on this repo rn (though I may become so in the future, esp. considering you just joined as maintainer). In any case, I've extracted the first commit into #702, as I indeed think that change is valuable on its own, and does make things easier to review here. |
@madsmtm I think it's okay to wait. Perhaps Servo should use |
Redo of #513, see that for previous discussion and motivation (GitHub Actions was giving me trouble).
I've focused on making this PR as minimal as possible, that is, I'm explicitly not trying to actually use any of the new features that
objc2
offers - we can discuss those separately as we get there. So as-is, this PR is only a small improvement on the status quo, though still definitely an improvement.The main difference you have to know for now is that all arguments and return types must implement a specific marker trait
objc2::encode::Encode
, which makes it possible to catch type mistakes at runtime, when debug assertions are enabled (e.g. cases of passing anu32
where anisize
was expected). This is a huge boon to soundness, and the few mistakes I've found and corrected are likely only the tip of the iceberg - I could use someone's help to run this on bigger code-bases that exercise more ofcocoa
/cocoa-foundation
.Feel free to ask any questions about anything!
CC recent contributors @jrmuizel, @mukilan, @jdm and @michaelwright235.