Skip to content
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

Add a mechanism to rerun bindgen with the same user options #2292

Merged
merged 2 commits into from
Nov 1, 2022

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Oct 3, 2022

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

@pvdrz pvdrz requested a review from emilio October 3, 2022 19:21
@pvdrz pvdrz changed the title Add an option to rerun bindgen with the same user options Add a mechanism to rerun bindgen with the same user options Oct 3, 2022
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.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

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 Show resolved Hide resolved
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(),
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 4, 2022

so @emilio the idea is that if bindgen finds a static inline function and the generate-static-inline options is enabled, it generates some extra c code that wraps the inline functions. So if you pass a file header.h to bindgen with the following contents:

static inline int foo() { return 42; }

then bindgen would generate an extra headers file header_inlined.h:

int foo_inlined()

with a corresponding header_inlined.c source file:

int foo_inlined() { return foo(); }

then Bindings::generate returns GenerateError::ShouldReturn { header: "header_inlined.h".into() }. Which means that the builder can include this new header file and run Bindings::generate again.

@bors-servo
Copy link

☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts.

@bors-servo
Copy link

☔ 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.
@pvdrz pvdrz marked this pull request as ready for review October 12, 2022 16:14
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.

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.

bindgen/lib.rs Outdated Show resolved Hide resolved
@pvdrz pvdrz merged commit a2fe04c into rust-lang:master Nov 1, 2022
@pvdrz pvdrz deleted the restart-button branch November 1, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants