-
Notifications
You must be signed in to change notification settings - Fork 747
Added Bindgen names to objective-c pointer return types for #1835. #1847
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
Added Bindgen names to objective-c pointer return types for #1835. #1847
Conversation
0e88711
to
6ffbdef
Compare
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.
Sorry for the lag reviewing this. This looks great, with one nit addressed. Though I guess I can also address it myself in a follow-up.
6ffbdef
to
22fa828
Compare
I rebased and addressed my nit, I'll merge once CI is back green. Thanks again for the fix! |
src/codegen/mod.rs
Outdated
@@ -3466,6 +3466,9 @@ impl TryToRustTy for Type { | |||
inner.into_resolver().through_type_refs().resolve(ctx); | |||
let inner_ty = inner.expect_type(); | |||
|
|||
let is_objc_pointer = | |||
matches!(inner_ty.kind(), TypeKind::ObjCInterface(..)); |
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.
@emilio This is much cleaner but wasn't added until 1.42 and the minimum supported Rust version is 1.34.. Thoughts?
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.
Oh, d'oh, I thought that we were using the matches
crate already... Sure, then let's make this just a plain match
:)
22fa828
to
7ee6eb6
Compare
* Took advantage of the repr transparent to use Bindgen return type names. * Updated unit tests and book
834014f
to
1431f2d
Compare
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'll address my nit in a follow-up.
@@ -3466,6 +3466,14 @@ impl TryToRustTy for Type { | |||
inner.into_resolver().through_type_refs().resolve(ctx); | |||
let inner_ty = inner.expect_type(); | |||
|
|||
let is_objc_pointer = | |||
inner.kind().as_type().map_or(false, |ty| { |
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.
You can use inner_ty
here, which is just a type
already, right?
This closes #1835.
This change seems too easy and really only works because the generated
structs
are#[repr(transparent)]
.I tested it with UIKit in my iced branch, and this is the changeset.
I also added some more documentation to highlight this change. Rendered.
Also, I'd like to mention that this is definitely a breaking change to any crates that take advantage of the objective-c generated bindings. After a little bit of searching on crates.io, I can't really find any published crates that do this. Perhaps I'm the only one?