Fix bugs in --clang-macro-fallback by jbaublitz · Pull Request #3072 · rust-lang/rust-bindgen (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits1 Checks94 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
I've tested the fixes, but would appreciate any additional feedback on this @ojeda.
33 tasks
I forgot to mention here that I'm doing some additional work on this so I changed it to a draft for the time being. I'll let you know when it's ready for review.
@pvdrz Is this CI failure related to the PR? It looks unrelated to me, but I'm not sure.
My other bindgen PR fails in the same way as this one, so I'm going to consider it unrelated. @ojeda I think this is ready for you to take a look at it if you'd like.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase, CI got fixed already IIRC.
Looks reasonable to me.
| .options |
|---|
| .clang_args |
| .iter() |
| .filter(|arg |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be starts_with? Also shouldn't we also skip the long-version of these? (--write-user-dependencies / --user-dependencies)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
This commit resolves a bug where -include was not respected and a bug where -MMD was passed to the fallback translation unit which would cause the dependency file to be overwritten with useless information about temporary files.
Would it be possible to add tests for these bugs?
Would it be possible to add tests for these bugs?
For the dependency generation, I am not sure how we would go about testing that only because I haven't seen any tests in the infrastructure for side effects, but it should be possible for the -include flag fix.