Sorting the output semantically #2254
Conversation
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
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.
Co-authored-by: Darren Kulp <[email protected]>
Co-authored-by: Darren Kulp <[email protected]>
|
thank you @kulp ! I committed the suggestions you made. I am ok with you squashing commits. |
|
☔ The latest upstream changes (presumably ebb1ce9) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
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.
Signed-off-by: Amanjeev Sethi <[email protected]>
…ted-code' into amanjeev/semantic-sorting-generated-code
…-sorting-generated-code
Signed-off-by: Amanjeev Sethi <[email protected]>
| .into_iter() | ||
| .map(|item| item.into_token_stream()); | ||
|
|
||
| return Ok(Bindings { |
There was a problem hiding this comment.
Ah, you need warnings here for this to compile. r=me with that.
There was a problem hiding this comment.
I was in the middle of adding those. May have to do that on Monday when I am back :)
There was a problem hiding this comment.
Solved (I cannot find the solved button for some odd reason)
| struct Point | ||
| { | ||
| number x; | ||
| number y; |
There was a problem hiding this comment.
Is this test testing much? It seems it's not using the new feature, so maybe remove it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@emilio should we remove it then?
There was a problem hiding this comment.
Either way is fine, sorry for the lag.
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 <[email protected]> Co-authored-by: Christian Poveda <[email protected]> Co-authored-by: Darren Kulp <[email protected]>
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 <[email protected]> Co-authored-by: Christian Poveda <[email protected]> Co-authored-by: Darren Kulp <[email protected]>
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
generatefunction. This feature however is hidden behind an optional flag--sort-semanticallywhich is defaulted tofalse. Hence, the existing generation is not affected if the flag is not used.Usage
Dependencies
synwas 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