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

Sorting the output semantically #2254

Merged
merged 13 commits into from Aug 24, 2022
Merged

Sorting the output semantically #2254

merged 13 commits into from Aug 24, 2022

Conversation

amanjeev
Copy link
Member

Issues affected

This PR closes #1743

Summary

This PR adds the ability to sort the output as requested in #1743. The sorting order of the output items is decided by a custom sort embedded within the generate function. This feature however is hidden behind an optional flag --sort-semantically which is defaulted to false. Hence, the existing generation is not affected if the flag is not used.

Usage

bindgen input.h -o output.rs --sort-semantically

Dependencies

syn was added as a dependency because it is used to enable this feature. After some discussion with folks here, it was suggested that incremental steps are preferred. Hence, this approach of re-generating if the flag is used.

Tests

A custom test was added to use the flag and a regular test was added for "unsorted" behaviour (current regular).

cc/ @pvdrz

pvdrz and others added 6 commits August 11, 2022 16:34
@pvdrz @amanjeev
add test for sorted items
588bbc5
@amanjeev
feat(lib sorting): add semantic sorting of the output
4344d99 
Generated code needs some sorting in a way that is
semantically appealing. The request[1] asks for basic
sorting like "ypes are declared first, then all structs,
then all consts, then all function signatures, etc.

[1] rust-lang#1743
@amanjeev
feat(lib sorting): test fix
29740bb
@amanjeev
feat(lib sorting): CLI options
8c45667
@amanjeev
feat(lib sorting): tests - sorted and unsorted
9eaba89 
We have this feature behind a feature flag - sort-semantically
whose default value is `false`.
* for sorted, a custom test is required in tests.rs - added
* for unsorted, regular builder, an unsorted header file is
enough.

If we make this feature default, we need to get rid of the
flag check and change all the header outputs because it will
break almost all tests.
@amanjeev
feat(lib sorting): comment explaining the hack
9a4714a
kulp
kulp approved these changes Aug 12, 2022
View reviewed changes
Copy link
Member

@kulp kulp left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.

I will be offline for the next week. If @emilio has not flagged something on this, I will revisit it after my return.

src/lib.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
amanjeev and others added 2 commits August 12, 2022 15:25
@amanjeev @kulp
feat(lib sorting): missing period/full stop at the end
f021cfd 
Co-authored-by: Darren Kulp <darren@kulp.ch>
@amanjeev @kulp
feat(lib sorting): typo
96b645e 
Co-authored-by: Darren Kulp <darren@kulp.ch>
@amanjeev
Copy link
Member Author

thank you @kulp ! I committed the suggestions you made. I am ok with you squashing commits.

@bors-servo
Copy link

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

emilio
emilio approved these changes Aug 18, 2022
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.

This looks great as an opt-in feature, thanks!

I just have one comment, I think the test could be simpler (i.e., not need a #[test] at all).

Also, if you want to describe the change in the CHANGELOG file it'd be awesome (because it saves me from doing that when cutting a release), but no big deal if not.

tests/tests.rs Outdated Show resolved Hide resolved
@amanjeev
feat(lib sorting): refactor test to use // bindgen-flags: method
e95764d 
Signed-off-by: Amanjeev Sethi <aj@amanjeev.com>
@amanjeev
Merge remote-tracking branch 'origin/amanjeev/semantic-sorting-genera…
315f017 
…ted-code' into amanjeev/semantic-sorting-generated-code
@amanjeev
Merge remote-tracking branch 'upstream/master' into amanjeev/semantic…
d99ca6d 
…-sorting-generated-code
@amanjeev
feat(lib sorting): CHANGELOG
a64c197 
Signed-off-by: Amanjeev Sethi <aj@amanjeev.com>
emilio
emilio approved these changes Aug 19, 2022
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.

r=me with those two changes (or without the test removal if I'm missing something).

src/lib.rs
.into_iter()
.map(|item| item.into_token_stream());

return Ok(Bindings {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you need warnings here for this to compile. r=me with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was in the middle of adding those. May have to do that on Monday when I am back :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Solved (I cannot find the solved button for some odd reason)

tests/headers/unsorted-items.h
struct Point
{
number x;
number y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test testing much? It seems it's not using the new feature, so maybe remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

One could argue that this is testing that not using --sort-semantically doesn't change the order. But in a sense this is tested with the rest of the test suite already as we didn't modify any expected rs file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emilio should we remove it then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way is fine, sorry for the lag.

This was referenced Aug 19, 2022
Open
Closed
@emilio emilio merged commit 6dfc3e7 into rust-lang:master Aug 24, 2022
@kulp kulp mentioned this pull request Sep 10, 2022
Merged
aatifsyed pushed a commit to aatifsyed/rust-bindgen that referenced this pull request Sep 22, 2022
@amanjeev @pvdrz @kulp
Sorting the output semantically (rust-lang#2254)
2d7dc08 
Generated code needs some sorting in a way that is
semantically appealing. The request[1] asks for basic
sorting like "types are declared first, then all structs,
then all consts, then all function signatures, etc.

[1] rust-lang#1743

Signed-off-by: Amanjeev Sethi <aj@amanjeev.com>
Co-authored-by: Christian Poveda <christian.poveda@ferrous-systems.com>
Co-authored-by: Darren Kulp <darren@kulp.ch>
qsdrqs pushed a commit to qsdrqs/rust-bindgen that referenced this pull request Oct 26, 2022
@amanjeev @pvdrz
@kulp @qsdrqs
Sorting the output semantically (rust-lang#2254)
860febe 
Generated code needs some sorting in a way that is
semantically appealing. The request[1] asks for basic
sorting like "types are declared first, then all structs,
then all consts, then all function signatures, etc.

[1] rust-lang#1743

Signed-off-by: Amanjeev Sethi <aj@amanjeev.com>
Co-authored-by: Christian Poveda <christian.poveda@ferrous-systems.com>
Co-authored-by: Darren Kulp <darren@kulp.ch>
@amanjeev amanjeev deleted the amanjeev/semantic-sorting-generated-code branch January 13, 2023 17:01
LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
@amanjeev @pvdrz
@kulp @LoganBarnett
Sorting the output semantically (rust-lang#2254)
4f7ee81 
Generated code needs some sorting in a way that is
semantically appealing. The request[1] asks for basic
sorting like "types are declared first, then all structs,
then all consts, then all function signatures, etc.

[1] rust-lang#1743

Signed-off-by: Amanjeev Sethi <aj@amanjeev.com>
Co-authored-by: Christian Poveda <christian.poveda@ferrous-systems.com>
Co-authored-by: Darren Kulp <darren@kulp.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

@pvdrz pvdrz pvdrz left review comments

@kulp kulp kulp approved these changes

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

Successfully merging this pull request may close these issues.

Request: sort all items emitted based on their item type
5 participants
@amanjeev @bors-servo @kulp @emilio @pvdrz