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

Remove now unneeded use statements for the glib_wrapper! macro #671

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
2 participants
@sdroege
Copy link
Member

sdroege commented Nov 27, 2018

See gtk-rs/glib#398

This is not complete. There's still the std::ptr added from src/analysis/out_parameters.rs:analyze_imports(). It seems like the condition there is wrong but I don't know what it should be. It seems to be related to src/codegen/function_body_chunk.rs:c_type_mem_mode_lib().

@EPashkin any ideas?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 27, 2018

IMHO all in analyze_imports is good.
It used for in functions like https://github.com/gtk-rs/gtk/blob/master/src/auto/builder.rs#L123-L129

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 27, 2018

It's the only place that adds std::ptr and in gstreamer (e.g. GstClock) it's added without being used

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 27, 2018

Maybe there commented function with out parameter?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 27, 2018

Found one problem function gst_clock_id_wait.
We don't checks for Type::Alias here in analyze_imports.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 27, 2018

But also caller-allocates="0" for gint64 out parameter is IMHO wrong anyway,
there also at minimum gst_clock_add_observation with out double that also have that mark,
so maybe gir don't need change for this part,

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 28, 2018

There some wrong deletion or better to say we don't add some use in function etc.
glib: src/auto/datetime.rs use ffi as glib_ffi;
gio: auto/drive.rs use std::ptr;
auto/icon.rs use glib_ffi;
auto/volume.rs use std::ptr;
atk: auto/rectangle.rs use gobject_ffi;
auto/text_range.rs use gobject_ffi;
gdk: auto/event_sequence.rs use gobject_ffi;

atk's and gdk's maybe problematic, as come from copy => |ptr| gobject_ffi:: part

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 28, 2018

So what do you suggest for fixing the GStreamer clock part exactly? The check there is apparently wrong but it's not clear to me what the correct one would be :)

For the deletion breaking things in other places, that means that the code generator needs to re-add the use statements in the other places where it actually makes use of the modules. I'll look into that at later.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 28, 2018

I checked Gtk-3.0.gir: seems it normal to use caller-allocates="0" for normal types :(
Then we have simply solution: in https://github.com/gtk-rs/gir/blob/master/src/analysis/out_parameters.rs#L124 calculate ConversionType and add import if it Direct or Scalar, no special case for Type::Alias needed.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 28, 2018

  • add import if it NOT Direct or Scalar.

@sdroege sdroege force-pushed the sdroege:glib-wrapper-standalone branch from 39ebd8a to 0cf5840 Nov 28, 2018

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 28, 2018

Pushed a new version. This seems to solve all the compiler errors and also produces no warnings anymore for gstreamer, gstreamer-base, glib, gio and gtk at least.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 28, 2018

I'm not going to submit regens once this is merged btw, I plan some more changes that will require regens all over the place in the next days and because everything just builds fine currently it doesn't seem very urgent :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 28, 2018

@sdroege Only last errors in gdk and atk.
Agree that we don't need hurry with regens.

Errors can be fixed with something like:

+++ b/src/analysis/record.rs
@@ -8,6 +8,7 @@ use nameutil::*;
 use super::*;
 use super::imports::Imports;
 use super::info_base::InfoBase;
+use super::record_type::RecordType;
 use traits::*;
 
 #[derive(Debug, Default)]
@@ -77,6 +78,13 @@ pub fn new(env: &Env, obj: &GObject) -> Option<Info> {
     let mut imports = Imports::with_defined(&env.library, &name);
     imports.add("glib::translate::*", None);
     imports.add("ffi", None);
+    if use_boxed_functions {
+        imports.add("gobject_ffi", None);
+    } else if record.glib_get_type.is_some() {
+        if let RecordType::AutoBoxed = RecordType::of(&record) {
+            imports.add("gobject_ffi", None);
+        }
+    }
 
     let mut functions = functions::analyze(
         env,
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 28, 2018

.. was slightly wrong, glib_get_type check need be outer if

@sdroege sdroege force-pushed the sdroege:glib-wrapper-standalone branch from 0cf5840 to 8b45d91 Nov 28, 2018

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 28, 2018

Next try :) This works in atk and gdk for me

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 29, 2018

This patch removes last warning in atk/gdk.

diff --git a/src/analysis/functions.rs b/src/analysis/functions.rs
index 90f9c4f..f6e57d1 100644
--- a/src/analysis/functions.rs
+++ b/src/analysis/functions.rs
@@ -362,6 +362,7 @@ fn analyze_function(
         if ret.base_tid.is_some() {
             imports.add("glib::object::Downcast", None);
         }
+        imports.add("glib::translate::*", version);
         bounds.update_imports(imports);
     }
 
diff --git a/src/analysis/record.rs b/src/analysis/record.rs
index f7fbaf5..796f309 100644
--- a/src/analysis/record.rs
+++ b/src/analysis/record.rs
@@ -76,7 +76,6 @@ pub fn new(env: &Env, obj: &GObject) -> Option<Info> {
     let use_boxed_functions = obj.use_boxed_functions;
 
     let mut imports = Imports::with_defined(&env.library, &name);
-    imports.add("glib::translate::*", None);
     imports.add("ffi", None);
     if record.glib_get_type.is_some() {
         if use_boxed_functions {
Remove now unneeded use statements for the glib_wrapper! macro
Also get rid of some unused use statements from GIO async functions that
are only enabled with a specific feature flag.

Thanks also to Evgenii Pashkin for providing various fixups on top of my
changes.

@sdroege sdroege force-pushed the sdroege:glib-wrapper-standalone branch from 8b45d91 to dae8416 Nov 29, 2018

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 29, 2018

Thanks, done! Should be good to go now :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 29, 2018

Ok, lets merge then.

@EPashkin EPashkin merged commit 6e9eca0 into gtk-rs:master Nov 29, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.