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: only generate trait signatures once #1461

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

jf2048
Copy link
Member

@jf2048 jf2048 commented Apr 28, 2023

Is there some reason this is not done already? It seems to reduce code size in gtk4-rs by about 5-6%.

@sdroege
Copy link
Member

sdroege commented Apr 28, 2023

I don't know why it is done this way. It was like that when I first looked at gtk-rs :) @GuillaumeGomez ?

@GuillaumeGomez
Copy link
Member

I don't remember at all. It looks like a failed rebase. If it doesn't change anything, then it's a great improvement!

@GuillaumeGomez GuillaumeGomez merged commit 8986bcf into gtk-rs:master Apr 28, 2023
@sdroege
Copy link
Member

sdroege commented Apr 28, 2023

What do you mean with failed rebase?

Also when regenerating, this will require changes to all the manual traits too.

@GuillaumeGomez
Copy link
Member

Then I clearly misunderstood what it was doing. Oh well.

@sdroege
Copy link
Member

sdroege commented Apr 28, 2023

It removes the ext trait function implementations and moves them into the trait definition as default implementations for the functions. So the ext trait implementation is now only a single line, and the signatures for the functions have to provided only a single time.

The main difference here is that if it's possible to also implement the trait on other types then they would automatically get the default implementations of the functions now instead of having to provide some. The default implementations would likely be wrong for any other implementation. But can there be other implementations of those traits or would all possible implementations overlap with the blanket implementation?

@GuillaumeGomez
Copy link
Member

So the show method for WidgetExt default impl would be working only for Widget and not for any other types implementing WidgetExt is what you meant?

@sdroege
Copy link
Member

sdroege commented Apr 28, 2023

It would work for T: IsA<Widget> but not for i32. But can someone implement the trait for i32?

@jf2048
Copy link
Member Author

jf2048 commented Apr 28, 2023

I think it makes sense to say we don't want anybody implementing WidgetExt etc for random types, maybe even seal those traits

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.

None yet

4 participants