-
Notifications
You must be signed in to change notification settings - Fork 790
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
emilio
left a comment
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
| 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
emilio
left a comment
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.
| 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
structsare#[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?