Fix macos expectations #2176
Fix macos expectations #2176
Conversation
* Updated tests/expectations/Cargo.toml to use 2018 rust. * Added Debug and Copy to objective-c structs. * Fixed lifetimes in objective-c trait templates. * Fixed imports for objective-c expectations tests.
simlay
force-pushed
the
fix-macos-expectations
branch
from
bb7fd33
to
fcb3cc3
Compare
| @@ -154,6 +159,9 @@ | |||
|
|
|||
| ## Security | |||
|
|
|||
|
|
|||
| [(#2176)]: https://github.com/rust-lang/rust-bindgen/pull/2176 | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, I mostly just was putting the "links" section at the end of that unreleased release section.
There was a problem hiding this comment.
Oh, right. Rendered, the markdown would not show anything under Security. My mistake.
| @@ -1,4 +1,4 @@ | |||
| // bindgen-flags: --objc-extern-crate -- -x objective-c | |||
There was a problem hiding this comment.
Is --objc-extern-crate still needed under any circumstances ? Since it no longer appears in any expectations, should we be thinking about removing the option, or else about re-adding expectations for it ?
There was a problem hiding this comment.
I'm up to remove it. I'd only argue that it should be an outside PR/issue to this. The main reason I removed it from all the tests expectations is because all of those expectations are modules and modules can't have #[macro_use] extern crate objc as that goes in a top level src/lib.rs/src/main.rs of a rust crate for rust 2015 edition crate.
There was a problem hiding this comment.
Though I have not tried to understand the flag itself, I agree that removing it would be out of scope for this PR. As long as we do not forget about the flag's presence, it seems fine not to deal further with it here.
| @@ -4381,7 +4381,7 @@ pub mod utils { | |||
| } | |||
| } else { | |||
| quote! { | |||
| use objc; | |||
| use objc::{self, msg_send, sel, sel_impl, class}; | |||
There was a problem hiding this comment.
Are we worried about namespace pollution in the generated code with generic names like class and self ? This may be of no importance – I honestly do not have a good handle on the implications of bringing in these names unqualified.
There was a problem hiding this comment.
I will admit that I don't know if self is needed here. It was there before so I left it in now that you question it, it may not be needed. The (unfortunate) generic class however is needed for the non-objc_extern_crate scenario.
Before rust edition 2018 (if I recall the history), things like:
#[macro_use] extern crate objc;were required for all external crates and imported all macros from that crate via the #[macro_use] annotation. With rust 2018, one doesn't need extern crate foo for crate foo on src/lib.rs or src/main.rs but as a result one cannot just blanket import all the macros (this is probably for the best).
One unfortunate part of this is that macros use macros but the macros don't import the macros from the crates they use that I know of and not from the crates they live in (for the objc crate anyway).
Anyway, I bring this up because I find the #[macro_use] annotation nice but only because it works now. If I'm being honest this crate should probably deprecate it but argue that it's outside the scope of this PR.
There was a problem hiding this comment.
I agree that deprecation is out of scope for this PR. I am pretty weak on the edition differences and macro scoping rules, but after your explanation I think the namespace collision problem is not worse than it was. Thanks.
haha. My bad! |
There was a problem hiding this comment.
I confirm that the tests below pass on my M1 Macbook with macOS 12.2.1 and rustc 1.59.0, whereas the tests/expectations directory fails on master, as expected.
for d in . tests/expectations tests/quickchecking bindgen-integration
do
(cd $d && cargo test)
doneLooks good to me. Thanks!
Closes #1954