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

Add async method code generation #490

Merged
merged 7 commits into from Nov 26, 2017
Merged

Conversation

antoyo
Copy link
Member

@antoyo antoyo commented Nov 12, 2017

For now, it's usable (tested with gio, gtk and sourceview), but we could improve the heuristic as mentionned in the TODO comment.

The other two things that will be implemented in next PRs will be the annotation to specify what is the finish function when it cannot be guessed and writing Unimplemented next to Cancellable when this type does not exist (i.e. when the user forgets to add it to Gir.toml).

I believe some code should be improved, especially the trampoline code generation.
If you have any other improvements, I'd be glad to hear them.

@antoyo
Copy link
Member Author

antoyo commented Nov 12, 2017

) -> (Option<String>, Option<String>) {
let type_name = bounds_rust_type(env, par.typ);
let mut type_string =
if async {
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you could write it like this:

if async && async_param_to_remove(&par.name) {
    return (None, None);
} else {
    type_name.into_string()
}

info_for_next_type: false,
});
return true;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This else condition is unnecessary since the if returns.

@@ -196,18 +236,20 @@ impl Bounds {
pub fn get_parameter_alias_info(&self, name: &str) -> Option<(char, BoundType)> {
self.used
.iter()
.find(move |n| if n.parameter_name == name {
.find(move |n| {
if n.parameter_name == name {
!n.info_for_next_type
Copy link
Member

Choose a reason for hiding this comment

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

The indent is broken in here. :p

};

if async_func {
Copy link
Member

Choose a reason for hiding this comment

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

if async_func && async_param_to_remove(&par.name) {
    add_rust_parameter = false;
}

) -> Result {
use library::Type::*;
let type_ = env.library.type_(type_id);
let rust_type = rust_type_full(env, type_id, nullable, ref_mode);
match *type_ {
Fundamental(super::library::Fundamental::Pointer) if async => Ok("TODO4".to_string()), // FIXME: remove
Copy link
Member

Choose a reason for hiding this comment

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

Wut?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, some TODO that are not generated.
I'll have to figure out why.

@GuillaumeGomez
Copy link
Member

What I'm seeing for the moment seems correct (even if it seems incomplete). However I'm not the maintainer in here hehe.

@sdroege
Copy link
Member

sdroege commented Nov 12, 2017

Can you create a PR for gio with the changes this would add, i.e. all the implementations for the async methods in GFile?

.map(|param| (param, type_mem_mode(env, param)))
.map(|(param, mode)| format!("let mut {} = {};\n", param.name, mode_to_string(mode)))
.collect::<Vec<_>>()
.join(" ");
Copy link
Member

Choose a reason for hiding this comment

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

Please change it to "\t\t\t\t"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll instead change that to use Chunk directly, to avoid having code as strings.

Ok(({result}))
}} else {{
Err(from_glib_full(error))
}};
Copy link
Member

Choose a reason for hiding this comment

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

IMHO let result = better be oneliner.

if id == namespaces::MAIN {
return "ffi".to_string();
}
format!("{}_ffi", name)
Copy link
Member

Choose a reason for hiding this comment

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

Namespace.ffi_crate_name is not same?

Copy link
Member Author

Choose a reason for hiding this comment

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

ffi_crate_name is only available on the other Namespace struct.

@EPashkin
Copy link
Member

@antoyo Overall is good idea, I slightly not like "internal trampoline" but it not so big problem. Thanks.

@antoyo
Copy link
Member Author

antoyo commented Nov 13, 2017

@sdroege I'll do it when I get back to Canada, so maybe in one week.
@EPashkin I didn't know how to do it (not sure if I can reuse the trampoline logic), so I did it this way because I wanted to have something working during the HackFest.
But the overall trampoline code generation should be redone before we merge this PR.

Thanks to all of you for your comments.

@EPashkin
Copy link
Member

@antoyo don't worry about trampolines.

@antoyo
Copy link
Member Author

antoyo commented Nov 22, 2017

@sdroege Here's the PR.

@antoyo
Copy link
Member Author

antoyo commented Nov 23, 2017

I've fixed all the issues you mentioned and I've fixed the async trampoline code generation.
I think this PR is ready to be tested.
Do we have any infrastructure to test gir against every GNOME library that we support?

@EPashkin
Copy link
Member

@antoyo No we don't have :(
I tested building of examples on updated gtk, it builds fine.

@EPashkin
Copy link
Member

@GuillaumeGomez we add async function it this release?

@sdroege
Copy link
Member

sdroege commented Nov 23, 2017

I added some comments about the generated code here gtk-rs/gio#63

@sdroege
Copy link
Member

sdroege commented Nov 25, 2017

IMHO this is good to go, judging from the generated code.

@GuillaumeGomez
Copy link
Member

Seems good to me as well. Is it good for you @EPashkin or do you want to add something else @antoyo?

@EPashkin
Copy link
Member

@GuillaumeGomez Its good for me too.
You wan't add this before release?

@GuillaumeGomez
Copy link
Member

Yes if possible so I'll handle the release tomorrow as the latest (won't be able to do it before next week-end otherwise).

@antoyo
Copy link
Member Author

antoyo commented Nov 25, 2017

@sdroege @GuillaumeGomez I wanted to fix the Cancellable issue mentionned by @sdroege.
I think I can do it today.

@antoyo
Copy link
Member Author

antoyo commented Nov 25, 2017

This will require to add Gio.Cancellable in Gir.toml files when needed.
I'll commit the fix soon.

@EPashkin
Copy link
Member

Gtk-rs examples still builds.
@sdroege, @antoyo there still any problem? Or I can merge?

@GuillaumeGomez
Copy link
Member

For me it was already good so as you wish!

@antoyo
Copy link
Member Author

antoyo commented Nov 25, 2017

It's ready to be merged.
Just waiting for @sdroege to see the latest change (&&Fn).

@sdroege
Copy link
Member

sdroege commented Nov 25, 2017

See my comment there

@antoyo
Copy link
Member Author

antoyo commented Nov 25, 2017

I've fixed this issue in the latest commit.

@antoyo
Copy link
Member Author

antoyo commented Nov 25, 2017

@sdroege I've fixed the other issues mentionned in the gio PR.

@EPashkin
Copy link
Member

@antoyo Thanks.
I add regen PR for sourceview. Gtk, glib has only commented changes and IMHO can be ignored.

@EPashkin EPashkin merged commit 24099bb into gtk-rs:master Nov 26, 2017
@EPashkin EPashkin mentioned this pull request Nov 26, 2017
@EPashkin
Copy link
Member

Actually gtk also have async functions in IconInfo, but current version not works good outside gio as it don't add use gio_ffi

@EPashkin
Copy link
Member

Created fix, will add regen for Gtk too.

@EPashkin EPashkin mentioned this pull request Nov 26, 2017
@sdroege
Copy link
Member

sdroege commented Nov 26, 2017

Looks good to me, just need to add support for the "progress-callback" pattern in the future, as used by various async operations

vhdirk pushed a commit to vhdirk/gir that referenced this pull request Jul 6, 2018
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