Skip to content

Permit IntKind::Custom to represent Paths instead of just Idents #1800

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

Merged
merged 3 commits into from
Jun 20, 2020

Conversation

eggyal
Copy link
Contributor

@eggyal eggyal commented Jun 10, 2020

If one implements ParseCallbacks::int_macro to specify a custom IntKind, one is currently restricted to mere Idents which must therefore be brought into the generated bindings' scope; this PR permits paths to be specified instead.

r? @emilio

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@emilio
Copy link
Contributor

emilio commented Jun 10, 2020

Looks fair, but could you add a test for this to the bindgen-integration directory?

@eggyal
Copy link
Contributor Author

eggyal commented Jun 10, 2020

The actual use-case here is likely to be no_std crates which have to provide their own equivalent of std::os::raw—but the bindgen-integration crate is not currently no_std; should I change that just for this test, or do something rather contrived (e.g. wrap c_void in a newtype struct, and export it together with the std types from a new module)? Else I'm not sure how this can be tested?

@emilio
Copy link
Contributor

emilio commented Jun 14, 2020

That use case should already be solved by --use_core and --ctypes-prefix, shouldn't it?

The actual use-case here is likely to be no_std crates which have to provide their own equivalent of std::os::raw—but the bindgen-integration crate is not currently no_std; should I change that just for this test, or do something rather contrived (e.g. wrap c_void in a newtype struct, and export it together with the std types from a new module)? Else I'm not sure how this can be tested?

It should be pretty easy to test without making the crate no_std, just make a module in the crate and use it for a particular macro?

@eggyal eggyal force-pushed the custom-intkind-paths branch from f7addbb to d8ed51e Compare June 15, 2020 07:38
@eggyal
Copy link
Contributor Author

eggyal commented Jun 15, 2020

I'm not entirely sure know what I was thinking—must have been a bad day. Will this suffice?

@@ -59,6 +59,13 @@ impl ParseCallbacks for MacroCallback {
_ => {}
}
}

fn int_macro(&self, _name: &str, _value: i64) -> Option<IntKind> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We somehow need to check this is doing the right thing. What about making this conditional only on _name (so use it for a single macro), and then use it in a #[test] function below like let _: isize = MY_MACRO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opted to verify the type via Any::is instead, so that failures will still compile (albeit fail the assertion).

@eggyal eggyal force-pushed the custom-intkind-paths branch from d8ed51e to ac17fc0 Compare June 15, 2020 20:55
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@emilio emilio merged commit 2ba27bf into rust-lang:master Jun 20, 2020
@eggyal eggyal deleted the custom-intkind-paths branch June 20, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants