-
Notifications
You must be signed in to change notification settings - Fork 707
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
Support complex macros. #2369
base: main
Are you sure you want to change the base?
Support complex macros. #2369
Conversation
ec87f2c
to
041af23
Compare
☔ The latest upstream changes (presumably 5875949) made this pull request unmergeable. Please resolve the merge conflicts. |
a159208
to
68ec633
Compare
Can someone please take a look at this PR? It's been over three months without a review. |
👋 Hi! Sorry for not giving you an answer in all this time. Somehow I missed this and wasn't aware of it. I will properly review this PR as I think it might solve many issues regarding the type that is assigned to these define macros when they get translated as constants. Bindgen has depended for a long time on That being said, we still should figure out the logistics of maintenance due to bindgen using your crate and I don't have the whole picture in my head so I'll defer that part to @emilio |
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 done with the review 🎉
I apologize in advance if I'm being harsh on some comments. I am really interested on understanding how all of this works 😁
bindgen-integration/Cargo.toml
Outdated
@@ -2,6 +2,7 @@ | |||
name = "bindgen-integration" | |||
description = "A package to test various bindgen features" | |||
version = "0.1.0" | |||
edition = "2018" |
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.
do we need to set this edition here? what's not working without it?
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.
Just checked. Exporting macros as items (pub __cmacro_MACRO as MACRO;
) is not supported without it.
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.
Hmm... I think we might need to document this edition restriction somewhere. Not sure what's bindgen
position about this but I could imagine someone having a code breakage due to certain macros now being supported.
@reitermarkus, this is really exciting because it would resolve a downstream bug I ran into with |
@overhacked, thanks for testing.
No idea when this will be merged, I already split some of the changes into separate PRs to make them easier to review, but all of them seem to also have stalled. |
Any update from the bindgen team on when this will get merged? |
This supersedes #2268 which aimed to support casts in macros.
The
cmacro
crate I developed over the last months allows parsing casts in variable-like macros and even more complex macros, such as function-like macros and nested macros.Macro parsing has been split from
Var
into a separateMacroDef
type, since macros need to be parsed last. Variables can be evaluated at parse time, macros on the other hand can only be fully evaluated after all types, variables, functions and other macros have been parsed. This allows getting the correct final value and is needed for parsing nested macros.