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

codegen: Use async keyword for not in traits functions #1482

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bilelmoussaoui
Copy link
Member

@bilelmoussaoui bilelmoussaoui commented Jul 9, 2023

Keep the Pin<Box> only for in trait async functions. Reduces the generated code a little bit :)

gtk4-rs PR: gtk-rs/gtk4-rs#1421

Keep the Pin<Box<T>> only for in trait async funcitons.
Reduces the generated code a little bit :)
async_future.success_parameters, error_parameters
)
} else {
if in_trait {
Copy link
Member

Choose a reason for hiding this comment

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

gtk-rs/gtk4-rs#1421 (comment) FWIW. This will now create differently behaving API depending on whether it's in a trait or not

Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Generally looks good but I don't like that the API behaves different between traits and non-traits, and also that it's "shape" is different.

Maybe for traits we can make use of the async-trait attribute macro for the time being then?

@bilelmoussaoui
Copy link
Member Author

Maybe for traits we can make use of the async-trait attribute macro for the time being then?

That is a possibility yes, I can find some time for that later.

@sdroege
Copy link
Member

sdroege commented Jul 10, 2023

That is a possibility yes, I can find some time for that later.

Should make this PR only simpler, I guess :)

@bilelmoussaoui
Copy link
Member Author

Now that async traits are stable, should we re-consider this and bump the minimal required rust version?

@sdroege
Copy link
Member

sdroege commented Jan 18, 2024

Not yet, see the announcement of the stabilization of the feature. It explicitly asked people to wait using it for public API for various reasons.

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.

2 participants