-
Notifications
You must be signed in to change notification settings - Fork 13.7k
initial implementation of the darwin_objc unstable feature #145660
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_hir/src/attrs |
This comment has been minimized.
This comment has been minimized.
I looked through the attribute changes and they look good to me. I can't judge the rest. @tmandry feel free to approve in both our names once you agree with the rest :) |
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.
Looked through it, it seems to match what I tried to do with the objc2
macros those years back (which is not to say that it is correct, I had no idea what I was doing back then).
Also, it would be helpful with more comments, and a few codegen tests - I get why you haven't done that yet, it's bothersome while we're still somewhat discussing what the best approach is.
Note: I'm still a bit unsure that this is actually the correct approach going forward, I suspect there might be more value in the future from something like the define_in_every_cgu_used
you first proposed?
At the very least we'll also need some way to get protocol references, we might also need metaclass references, and possibly more if we're to support static NSString and support fully statically declared classes.
But I think it's fine to move forwards with this in the current state, then we can always figure that sort of stuff out well before stabilisation.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I added |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #145728) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I added a lot of tests. Let me know if there's any other tests you'd like to see. I took a peek at adding |
This comment has been minimized.
This comment has been minimized.
@madsmtm Is there anything else you'd like me to address? |
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 like the objc_abi_version
method you added, that makes things easier to read.
invariant.load
That's fine, and I agree it seems like it's just an optimization anyhow (though possibly an important one for e.g. loop-invariant code motion).
I think I'm comfortable enough with the compiler parts to approve that (so r=me for that part).
I'm a bit unsure how we do this process-wise, but I think this needs a t-libs reviewer as well for the library parts? (even though that part is relatively minimal) And if I'm reading the docs right, I think @tmandry needs to start a lang FCP.
Let's see how the tests fare in the meantime:
@bors try jobs=*apple*
This comment was marked as resolved.
This comment was marked as resolved.
@bors try |
initial implementation of the darwin_objc unstable feature try-job: `*apple*`
This comment has been minimized.
This comment has been minimized.
💔 Test for 19b63b3 failed: CI. Failed jobs:
|
This comment has been minimized.
This comment has been minimized.
@bors try |
This comment has been minimized.
This comment has been minimized.
initial implementation of the darwin_objc unstable feature try-job: `*apple*`
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
@bors try |
@jbatez: 🔑 Insufficient privileges: not in try users |
@bors try |
initial implementation of the darwin_objc unstable feature try-job: `*apple*`
Tracking issue: #145496
This feature makes it possible to reference Objective-C classes and selectors using the same ABI used by native Objective-C on Apple/Darwin platforms. Without it, Rust code interacting with Objective-C must resort to loading classes and selectors using costly string-based lookups at runtime. With it, these references can be loaded efficiently at dynamic load time.
r? @tmandry
try-job:
*apple*