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

Change Callbacks API. #2766

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Feb 21, 2024

Extracted from #2369 for easier review.

Also apply #[allow(unused_variables)] everywhere to show nicer argument names in docs.

@reitermarkus reitermarkus force-pushed the int-macro-i128 branch 4 times, most recently from de17fc8 to aba0479 Compare February 21, 2024 17:25
bindgen/ir/var.rs Outdated Show resolved Hide resolved
@reitermarkus reitermarkus force-pushed the int-macro-i128 branch 2 times, most recently from 8b65527 to 0600260 Compare February 21, 2024 19:06
@@ -2159,8 +2159,8 @@ If you encounter an error missing from this list, please file an issue or a PR!"
let mut kind = ModuleKind::Normal;
let mut looking_for_name = false;
for token in cursor.tokens().iter() {
match token.spelling() {
b"inline" => {
match token.spelling().to_str().unwrap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What guarantees that the tokens are utf-8? I think the previous code was fine.

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 already assume UTF-8 in a bunch of other places, e.g. macro names and variable names. Granted here it's more for consistency rather than necessity.

let mut tokens = tokens
.iter()
.map(|token| token.spelling().to_str().unwrap())
.collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions, I'd rather avoid crashing for non-utf8 input and just expose bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is simply extracted from #2369 in order to be able to use cmacro.

cmacro needs to assume a single encoding to correctly parse macros. The same is true for users that implement ParseCallbacks::fn_macro, they cannot just guess the encoding without knowing which file the byte slice came from.

I guess we can just skip macros containing invalid UTF-8 for now, though.

@reitermarkus reitermarkus force-pushed the int-macro-i128 branch 2 times, most recently from 8178f51 to ba72f63 Compare February 28, 2024 15:52
@reitermarkus
Copy link
Contributor Author

@emilio, changed the code to skip non-UTF-8 items instead of crashing.

bindgen/callbacks.rs Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Contributor Author

@emilio, can you have a look here again? Thanks.

@bors-servo
Copy link

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

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

4 participants