-
Notifications
You must be signed in to change notification settings - Fork 742
Add a mechanism to rerun bindgen with the same user options #2292
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
Conversation
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'm a bit confused about the ShouldRestart
code, can you elaborate?
Generally looks good, with some nits.
src/clang.rs
Outdated
@@ -1823,7 +1823,7 @@ pub struct UnsavedFile { | |||
|
|||
impl UnsavedFile { | |||
/// Construct a new unsaved file with the given `name` and `contents`. | |||
pub fn new(name: &str, contents: &str) -> UnsavedFile { | |||
pub fn new(name: String, contents: String) -> UnsavedFile { |
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.
Why this change?
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.
We can pass name
and contents
by value to UnsavedFile::new
which means that the calls to CString::new
don't have to clone name
and contents
src/lib.rs
Outdated
Bindings::generate(self.options, input_unsaved_files) | ||
match Bindings::generate(options, input_unsaved_files) { | ||
Ok(bindings) => Ok(bindings), | ||
Err(GenerateError::ShouldRestart) => self.generate(), |
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.
Why? I don't get this. This is dead code and would trivially loop right?
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.
actually let me do a change that could make this clearer.
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.
see #2292 (comment)
so @emilio the idea is that if bindgen finds a static inline function and the static inline int foo() { return 42; } then bindgen would generate an extra headers file int foo_inlined() with a corresponding int foo_inlined() { return foo(); } then |
☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #2302) made this pull request unmergeable. Please resolve the merge conflicts. |
This adds a mechanism so `bindgen` is able to run `Bindings::generate` multiple times with the same user input if the `generate_static_inline` option is enabled and `GenerateError::ShouldRestart` is returned by `Bindings::generate`. This is done to eventually solve rust-lang#1090 which would require to check for any static inline functions and generate a new header file to be used as an extra input and run `Bindings::generate` again.
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.
Looks ok / progress, but I'd probably merge this along with the feature using it? Anyways looks good to merge if you want to make progress in-tree.
In some cases, like when processing headers with static inline functions, might be useful to have a mechanism to rerun Bindgen with the same options the user provided.
I'm opening this PR as a first step to solve #1090 and not make a hard to review and huge PR. The idea would be to run bindgen a first time to find any inlined functions, then generate a new
.h
file that wraps the inlined functions and finally run bindgen again using this generated file as an input.Blocked by: #2302