Skip to content

Refactor Enum in codegen #676

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

Merged
merged 1 commit into from
May 5, 2024

Conversation

lilizoey
Copy link
Member

Like #673 but the codegen is kept as global functions and not inherent methods to Enum/Enumerator.

@lilizoey lilizoey added the quality-of-life No new functionality, but improves ergonomics/internals label Apr 25, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-676

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Several smaller changes are hidden in the moved-around blocks, I already commented on some. If there are some I've missed and you consider worth reviewing, please mention them 🙂

This will likely break #627, but as it stands, that PR doesn't have the highest chances to get merged (Godot decision still pending). So that's probably OK. And I'd expect potential rebase effort to be reasonable.

let mut enumerators = Vec::with_capacity(rust_enumerators.len());
quote! {
#[repr(transparent)]
#[derive(#( #derives ),*)]
Copy link
Member

Choose a reason for hiding this comment

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

Here I wonder if the whole #[derive(...)] attribute should be stored in a variable... Or do you see a benefit in having individual derives in a list?

For example, this approach would not work in case the #derives list is empty, whereas the other could emit an empty token-stream.

Copy link
Member Author

@lilizoey lilizoey May 3, 2024

Choose a reason for hiding this comment

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

Actually this would work if the #derives list is empty, since this is perfectly legal to do:

#[derive()]
struct Foo;

and i think the code is simpler if i dont store it in a variable.

@lilizoey lilizoey force-pushed the refactor/enum-codegen-2 branch 3 times, most recently from 294e187 to abd726f Compare May 3, 2024 16:57
@Bromeon Bromeon force-pushed the refactor/enum-codegen-2 branch from abd726f to b7f19bc Compare May 4, 2024 20:13
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Mostly good I think, 1-2 minor things left.

@lilizoey lilizoey force-pushed the refactor/enum-codegen-2 branch from b7f19bc to de223d0 Compare May 5, 2024 14:29
@lilizoey lilizoey marked this pull request as ready for review May 5, 2024 14:33
Copy link
Member

@Bromeon Bromeon 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!

@Bromeon Bromeon added this pull request to the merge queue May 5, 2024
Merged via the queue into godot-rust:master with commit 5d72b64 May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants