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

codgen: run the visitor on global functions as well #1313

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bilelmoussaoui
Copy link
Member

otherwise glib::Priority is not replaced for them and we have to manually implement them in such case
The same change is needed for Builder pattern

otherwise glib::Priority is not replaced for them and we have to manually implement them in such case
The same change is needed for Builder pattern
@bilelmoussaoui
Copy link
Member Author

Do not merge yet, I will handle other cases as well first :)

Currently, the generated builder pattern doesn't modify the priority properties to make use of glib::Priority.
We can't do something like what is done for functions parameters/returns as the builder properties are computed during
the analysis phase.
@@ -43,14 +43,16 @@ struct ReplaceToPriority {

impl FunctionsMutVisitor for ReplaceToPriority {
fn visit_function_mut(&mut self, func: &mut Function) -> bool {
if !func.name.ends_with("_async") {
Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly this change is not great because it ends up using the glib::Priority where it doesn't make sense. Well there was only one case in gtk3-rs and another in gtk4-rs. E.g, https://docs.gtk.org/gtk3/method.TextTag.set_priority.html. Although, it drops a bunch of manual code from gtk4-rs, could be a fair trade, not sure :)

Copy link
Member Author

Choose a reason for hiding this comment

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

will test it with gstreamer once the send on callbacks changes lands

@sdroege
Copy link
Member

sdroege commented Jan 25, 2022

Let me know when this is good for a review. I think changing every int that is called something with priority is not great, it will have many false positives. Maybe the functions have some other heuristic you could use? Or maybe we can have a configuration to change a parameter type to something else (don't we have that already?)?

@bilelmoussaoui
Copy link
Member Author

Let me know when this is good for a review. I think changing every int that is called something with priority is not great, it will have many false positives. Maybe the functions have some other heuristic you could use? Or maybe we can have a configuration to change a parameter type to something else (don't we have that already?)?

I think the best way going forward is to keep the detection automatic with a way to configure it per parameter/function return. It needs a bit more work for now

@bilelmoussaoui
Copy link
Member Author

@sdroege can you think of any other potential conversions we can configure like the priority one? Just thinking if we need some generic configuration like "disable_preprocess_visitor = false` or have something per type (probably unneeded)

@sdroege
Copy link
Member

sdroege commented Jan 25, 2022

I don't know, sorry

@bilelmoussaoui
Copy link
Member Author

We also have conversions from an int to gdk::Key type in gtk4-rs, all of those functions have to be manually implemented sadly. So maybe we need a generic solution here

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