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

Parse code by source order. #2543

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Jun 3, 2023

Extracted from #2369.

In order to somewhat support #undef, code needs to be parsed in the correct order. Previously, all macros were parsed before all variables, making it impossible to reason about whether a variable was declared before or after a macro with the same name.

@reitermarkus reitermarkus force-pushed the codegen-order branch 6 times, most recently from 56bd04e to ffe404a Compare June 3, 2023 10:16
@bors-servo
Copy link

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

@reitermarkus reitermarkus force-pushed the codegen-order branch 3 times, most recently from b2281c1 to 3ffbd90 Compare June 8, 2023 19:15
@reitermarkus reitermarkus changed the title Generate code based on source order. Parse code by source order. Jun 8, 2023
@reitermarkus reitermarkus force-pushed the codegen-order branch 5 times, most recently from e72bdf7 to b8ab27a Compare June 8, 2023 19:40
@pvdrz pvdrz linked an issue Jun 8, 2023 that may be closed by this pull request
&self,
other: &Self,
ctx: &BindgenContext,
) -> Option<cmp::Ordering> {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood correctly, the only case where this returns None is if neither the file is included in the other. Given that this is only called inside visit_sorted wouldn't it be clearer to just return cmp::Ordering::Equal in that case?

bindgen/clang.rs Show resolved Hide resolved
bindgen/clang.rs Show resolved Hide resolved
bindgen/codegen/mod.rs Outdated Show resolved Hide resolved
bindgen/ir/context.rs Show resolved Hide resolved
@pvdrz
Copy link
Contributor

pvdrz commented Jun 8, 2023

I just did a review and added a few comments, most of them are just about documentation.

Could you also please document these changes in the changelog. Mainly the fact that the order of the bindings has changed and that the __bindgen_ty_\d+ types might change name because of this?

Also I have a last question: The evaluation of define directives is not affected by this new ordering yet, right?

@reitermarkus reitermarkus force-pushed the codegen-order branch 3 times, most recently from f9fe4ed to 255bf14 Compare June 9, 2023 00:38
@reitermarkus
Copy link
Contributor Author

The evaluation of define directives is not affected by this new ordering yet, right?

No, this is simply extracting this logic from #2369 for easier review.

@pvdrz pvdrz merged commit 83f729f into rust-lang:main Jun 9, 2023
@reitermarkus reitermarkus deleted the codegen-order branch June 9, 2023 12:47
@emilio
Copy link
Contributor

emilio commented Jun 15, 2023

@reitermarkus fyi, this broke Firefox (still working on a more reduced test-case). I'm a bit confused about this change tho. You completely ignore the CXChildVisit_{Break,Continue,Recurse} return value of the visitor? I'm surprised this didn't break more stuff.

@emilio
Copy link
Contributor

emilio commented Jun 15, 2023

I guess none of the callers cared about the different CXChildVisit but at least the API should be simplified to reflect the fact that the return value is ignored...

@pvdrz
Copy link
Contributor

pvdrz commented Jun 15, 2023

Hmmm... this is unfortunate. I'll try to review all the issues/PRs now.

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.

Macro redefinitions are not being respected
4 participants