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.

Sign up for GitHub

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

Jump to bottom

Fix source order sorting. #2561

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

Fix source order sorting. #2561

wants to merge 11 commits into from

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Jun 16, 2023 •
edited

Fixes #2556.

Previously, headers included with -include were not added to the includes map since they don't have a source file. This is now fixed.

Also, some wrong match arms were removed/fixed.

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Jun 16, 2023 •
edited

cc @glandium @emilio

Please have a look.

reitermarkus
reitermarkus commented Jun 16, 2023
View reviewed changes
bindgen-tests/tests/tests.rs
.header_contents("test.h", "int bar(const char* a);")
.header_contents("test2.h", "float bar2(const char* b);")
.header(concat!(env!("CARGO_MANIFEST_DIR"), "/tests/headers/char.h"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This doesn't change anything, it simply reflects the order these are stored internally and eventually generated in.

@reitermarkus reitermarkus force-pushed the source-order branch 2 times, most recently from 5fe6d48 to 5c4cf4f Compare June 16, 2023 13:13
emilio
emilio reviewed Jun 16, 2023
View reviewed changes
bindgen/clang.rs Outdated Show resolved Hide resolved
bindgen/clang.rs Outdated Show resolved Hide resolved
@reitermarkus reitermarkus requested a review from emilio June 17, 2023 16:14
@bors-servo
Copy link

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

@reitermarkus reitermarkus force-pushed the source-order branch 3 times, most recently from e6a48a2 to 21d14df Compare June 17, 2023 21:09
reitermarkus
reitermarkus commented Jun 19, 2023
View reviewed changes
bindgen/clang.rs Outdated Show resolved Hide resolved
@reitermarkus reitermarkus force-pushed the source-order branch 4 times, most recently from 7fe4d02 to 79ad27e Compare June 20, 2023 19:44
@reitermarkus
Copy link
Contributor Author

I refactored SourceLocation to be an enum and the sorting is now split between SourceLocation and IncludeLocation. In my opinion it is a bit easier to follow that way.

@reitermarkus reitermarkus force-pushed the source-order branch 2 times, most recently from f287853 to 6553527 Compare June 20, 2023 20:30
@reitermarkus
Copy link
Contributor Author

@pvdrz, I fixed the edge case you gave and added a test for it.

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Jun 21, 2023 •
edited

I changed all file names from String to PathBuf since we need to absolutize them before comparing them, e.g. running your example with cargo run -- a.h would actually try to compare the strings "a.h" and "./a.h" before.

@reitermarkus reitermarkus requested a review from pvdrz June 22, 2023 17:17
emilio
emilio reviewed Jun 26, 2023
View reviewed changes
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.

I still think this is quite complicated for the benefit it brings fwiw... :/

@reitermarkus
Copy link
Contributor Author

@emilio, do you have an alternative? The status quo is simply incorrect in some cases.

The only alternative I can see is to never generate constants for variable-like macros and only ever generate Rust macros instead. That way there is never a naming or ordering conflict.

Note that this is also needed for the same reason for function-like macros and functions in #2369.

@emilio
Copy link
Contributor

emilio commented Jul 11, 2023

@reitermarkus I'm confused, my understanding is that this sorting fixes some cases but breaks others, is that not the case?

@reitermarkus
Copy link
Contributor Author

No, it only broke some cases due to the sorting being wrong/incomplete previously. Otherwise nothing should break, except for some auto-generated names having a different index.

This was referenced Aug 8, 2023
Open
Open
@alex
Copy link
Member

alex commented Aug 8, 2023

Confirmed that this works for one of my projects.

@reitermarkus
Copy link
Contributor Author

@pvdrz, @emilio, please have another look here, thank you.

@reitermarkus
Copy link
Contributor Author

@pvdrz, @emilio, another ping here.

@bors-servo
Copy link

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

@reitermarkus
Copy link
Contributor Author

@pvdrz, @emilio, can you have another look at this?

emilio
emilio reviewed Jan 13, 2024
View reviewed changes
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.

I still think we should have some measurements of the overhead of the source order sorting, and turn it off by default if it's non-trivial.

Some of the refactoring like refactoring to use the clang::SourceLocation enum seems nice. Though that said, I'd rather keep the CXFile rather than reading the path and doing IO to absolutize it eagerly for most callers that don't need that...

@reitermarkus
Copy link
Contributor Author

turn it off by default

This is a prerequisite for supporting complex macros (#2369), so it can't be optional.

@bors-servo
Copy link

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

@reitermarkus reitermarkus force-pushed the source-order branch 2 times, most recently from a1f48ff to a440b2b Compare February 10, 2024 00:27
@bors-servo
Copy link

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

@reitermarkus
Copy link
Contributor Author

@pvdrz, @emilio please have another look.

I'd rather keep the CXFile rather than reading the path and doing IO to absolutize it eagerly for most callers that don't need that...

I have re-added the File struct as SourceFile now to avoid eagerly absolutizing every path.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 3b5ce9c) 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
Reviewers

@pvdrz pvdrz Awaiting requested review from pvdrz

@emilio emilio Awaiting requested review from emilio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Types can end up in the wrong namespace
5 participants
@reitermarkus @bors-servo @emilio @alex @pvdrz