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

Use absolute paths for unsaved files passed to clang and prepend -inc… #1857

Closed
wants to merge 1 commit into from

Conversation

curldivergence
Copy link

…lude directives to them.

Fixes #1771

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.

Thank you, this looks great! Just a minor question about the test, and one nit that rustfmt has that you need to fix before merging.

assert_eq!(expected, actual);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

You need to remove this stray newline for CI to pass.


let expected = include_str!(concat!(
env!("CARGO_MANIFEST_DIR"),
"/tests/expectations/tests/test_mixed_header_and_header_contents.rs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, how do you update this file when output changes? Do you need to do that manually? If so, it's a bit unfortunate.

Copy link
Author

Choose a reason for hiding this comment

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

Hey, thanks for review :) And could you elaborate on this issue, please? I'm not sure what are the other ways to update the expectation but to do it manually, unfortunately. E.g., in test_multiple_header_calls_in_builder the pattern is the same, if I got it right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you're right that this pattern is not new, it's just worse having tests you need to manually update, because the overhead of updating them scales with the amount of tests :(

I guess we can take this as-is though.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, now that I think about it - I can use two existing test expectation files and try to concatenate them with inline expectation from header_contents calls, so that I will not introduce new expectation files.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds great. as long as the first header doesn't have any anonymous enum or struct it should work.

@emilio emilio closed this in 5177889 Jan 10, 2021
@bors-servo
Copy link

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

@emilio
Copy link
Contributor

emilio commented Jan 12, 2021

Very smart, bors.

LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
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.

mixing header and header_contents always panics
3 participants