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.

Sign up for GitHub

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

Jump to bottom

Support BoxedInline without a get_type function #1297

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Support BoxedInline without a get_type function #1297

wants to merge 6 commits into from

Conversation

wash2
Copy link

@wash2 wash2 commented Dec 29, 2021 •
edited

This is my attempt at supporting BoxedInline without a get_type function #1236 #1295. Generating the bindings for gmodule does not panic and now outputs:

[WARN  libgir::analysis::functions] Function g_module_symbol has unsupported outs
[WARN  libgir::analysis::functions] Function g_module_open_full has unsupported outs
[ERROR libgir::analysis::record] Missing memory management functions for GModule.Module

@wash2 wash2 changed the title Support BoxedInline without a get_type function #1295 #1236 Support BoxedInline without a get_type function Dec 29, 2021
sdroege
sdroege reviewed Dec 30, 2021
View reviewed changes
src/codegen/general.rs Outdated
sys_crate_name,
get_type_fn
)?;
if let Some((ref get_type_fn, _get_type_version)) = get_type_fn {
Copy link
Member

Choose a reason for hiding this comment

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

This requires a get_type function, or otherwise more changes in glib are needed

Copy link
Member

Choose a reason for hiding this comment

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

For the case of not BoxedInline but normal Boxed

Copy link
Author

Choose a reason for hiding this comment

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

So the get_type_fn here can't be None then?

Copy link
Member

Choose a reason for hiding this comment

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

Well, there won't be a copy/free function here otherwise and that won't compile without further changes in glib

sdroege
sdroege reviewed Dec 30, 2021
View reviewed changes
src/library.rs Outdated
@@ -1085,18 +1085,19 @@ impl Library {
}
}
if let Some(tid) = env.library.find_type(0, &full_name) {
let gobject_id = env.library.find_type(0, "GObject.Object").unwrap();
let gobject_id = env.library.find_type(0, "GObject.Object");
Copy link
Member

Choose a reason for hiding this comment

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

If you don't have GObject in your dependencies then there can't be any class hierarchy at all, so the code below should simply not run

Copy link
Author

Choose a reason for hiding this comment

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

Ok gotcha

sdroege
sdroege reviewed Dec 30, 2021
View reviewed changes
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.

Otherwise makes sense.

Why do you want to create bindings for GModule though? I think that's going to require writing the whole bindings manually to end up with anything usable and somewhat safe. The autogenerated bindings won't be usable.

@sdroege sdroege linked an issue Dec 30, 2021 that may be closed by this pull request
Panic on GIR not_bound mode for GModule #1295
Open
bilelmoussaoui
bilelmoussaoui reviewed Dec 30, 2021
View reviewed changes
src/codegen/record.rs Outdated
&analysis.init_function_expression,
&analysis.copy_into_function_expression,
&analysis.clear_function_expression,
&analysis.glib_get_type,
Copy link
Member

Choose a reason for hiding this comment

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

maybe analysis.glib_get_type.map(|ref a,_| a) or something like that to avoid passing the version that's unneeded here ?

Copy link
Author

Choose a reason for hiding this comment

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

Ok

@wash2
Copy link
Author

wash2 commented Dec 30, 2021

Why do you want to create bindings for GModule though? I think that's going to require writing the whole bindings manually to end up with anything usable and somewhat safe. The autogenerated bindings won't be usable.

I want to use GModule to support "plug-ins"
I was not aware that the auto-generated bindings would not be usable, but I could try manually writing safe bindings if necessary.

@sdroege
Copy link
Member

sdroege commented Dec 30, 2021

I was not aware that the auto-generated bindings would not be usable

The -sys bindings should be fine, the non-sys ones probably only have very few functions generated and you'll have to be very careful here with memory safety. Take a look at all the complications in the libloading API for example.

@wash2
Copy link
Author

wash2 commented Dec 30, 2021

I was not aware that the auto-generated bindings would not be usable

The -sys bindings should be fine, the non-sys ones probably only have very few functions generated and you'll have to be very careful here with memory safety. Take a look at all the complications in the libloading API for example.

Ok, thanks, I'll check that out

bilelmoussaoui
bilelmoussaoui reviewed Dec 30, 2021
View reviewed changes
Copy link
Member

@bilelmoussaoui bilelmoussaoui left a comment

Choose a reason for hiding this comment

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

Wouldn't this work ?

src/codegen/general.rs Outdated
@@ -377,7 +377,7 @@ pub fn define_auto_boxed_type(
init_function_expression: &Option<String>,
copy_into_function_expression: &Option<String>,
clear_function_expression: &Option<String>,
get_type_fn: &str,
get_type_fn: Option<&String>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
get_type_fn: Option<&String>,
get_type_fn: Option<&str>,

src/codegen/record.rs Outdated
&analysis.init_function_expression,
&analysis.copy_into_function_expression,
&analysis.clear_function_expression,
analysis.glib_get_type.as_ref().map(|(ref a, _)| a),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
analysis.glib_get_type.as_ref().map(|(ref a, _)| a),
analysis.glib_get_type.as_ref().map(|(ref a, _)| a).as_deref(),

Copy link
Author

Choose a reason for hiding this comment

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

Coercion still fails for me with as_deref()

@bilelmoussaoui bilelmoussaoui mentioned this pull request Jan 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdroege sdroege sdroege left review comments

@bilelmoussaoui bilelmoussaoui bilelmoussaoui left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Panic on GIR not_bound mode for GModule
3 participants
@wash2 @sdroege @bilelmoussaoui