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

Update code generation for glib_wrapper! macro and IsA<_>, etc cleanup #689

Merged
merged 14 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@sdroege
Copy link
Member

sdroege commented Jan 7, 2019

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from d48ba81 to 6950499 Jan 9, 2019

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 10, 2019

New version of macros still don't works well:

error[E0277]: the trait bound `auto::cancellable::Cancellable: glib::translate::ToGlibPtr<'_, *mut gobject_ffi::GObject>` is not satisfied
  --> D:/eap/rust/0/gio\src\auto\cancellable.rs:99:30
   |
99 |             connect_raw(self.to_glib_none().0, b"cancelled\0".as_ptr() as *const _,
   |                              ^^^^^^^^^^^^ the trait `glib::translate::ToGlibPtr<'_, *mut gobject_ffi::GObject>` is not implemented for `auto::cancellable::Cancellable`
   |
   = help: the following implementations were found:
             <auto::cancellable::Cancellable as glib::translate::ToGlibPtr<'a, *const ffi::GCancellable>>
             <auto::cancellable::Cancellable as glib::translate::ToGlibPtr<'a, *mut ffi::GCancellable>>

for unparented Cancellable and connect without " as *mut _":

    pub struct Cancellable(Object<ffi::GCancellable, ffi::GCancellableClass, CancellableClass>);

    pub fn connect_cancelled<F: Fn(&Cancellable) + Send + Sync + 'static>(&self, f: F) -> SignalHandlerId {
        unsafe {
            let f: Box_<Box_<Fn(&Cancellable) + Send + Sync + 'static>> = Box_::new(Box_::new(f));
            connect_raw(self.to_glib_none().0, b"cancelled\0".as_ptr() as *const _,
                transmute(cancelled_trampoline as usize), Box_::into_raw(f) as *mut _)
        }
    }

but with "*mut _" in can't infer type.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 10, 2019

New version of macros still don't works well:

Yes, that's the next thing on my list. Went down from 800+ compiler errors in gio to only 24 yesterday :) This is one of those. connect_raw() needs slightly different code generated, but that one is relatively easy to fix.

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from 6950499 to 9ac9ca5 Jan 10, 2019

Always generate IsA<T> bounds for classes even if they have no known …
…subclasses here

They might have subclasses in another crate!

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch 2 times, most recently from ca3788a to 9a3cfa3 Jan 10, 2019

sdroege added some commits Jan 10, 2019

Generate a typed None constant for references to Objects
This can be used in functions that take a Option<IsA<T>> for convenience
instead of passing None::<&T>.

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch 4 times, most recently from dc36d92 to 684604e Jan 14, 2019

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from 684604e to f98d2c1 Jan 14, 2019

@sdroege sdroege changed the title [WIP] Update code generation for glib_wrapper! macro and IsA<_>, etc cleanup Update code generation for glib_wrapper! macro and IsA<_>, etc cleanup Jan 14, 2019

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 14, 2019

This is now ready to be reviewed and merged. I have local branches for all other crates and it's all compiling fine.

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from f0da9c0 to 2e0fea9 Jan 14, 2019

@sdroege sdroege force-pushed the sdroege:is-a-cleanup branch from 2e0fea9 to d96589b Jan 14, 2019

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 14, 2019

And all compiling fine without new compiler warnings too

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 14, 2019

Yes, only one new warning in manual gio now:

warning: unused import: `glib::translate::*`
 --> D:/eap/rust/0/gio\src\subprocess_launcher.rs:6:5
  |
6 | use glib::translate::*;
  |     ^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

Thanks, all looks very good for me with one exception:
Using @ as prefix, looks not in rust style for me, but if we can use just words, then lets it be.
pub struct BufferedInputStream(Object<ffi::GBufferedInputStream, ffi::GBufferedInputStreamClass, BufferedInputStreamClass>) @extends FilterInputStream, InputStream, @implements Seekable;

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 14, 2019

@GuillaumeGomez I will merge this if you not against it.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 14, 2019

Yes, only one new warning in manual gio now:

warning: unused import: `glib::translate::*`
 --> D:/eap/rust/0/gio\src\subprocess_launcher.rs:6:5
  |
6 | use glib::translate::*;
  |     ^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(unused_imports)] on by default

Ah, I didn't see that one. Will fix that with the gio PR :)

Using @ as prefix, looks not in rust style for me, but if we can use just words, then lets it be.

I also don't like it much but we need the "keywords" to start with something that is not allowed at the beginning of a path. @ seemed like a nice compromise here compared to everything else I was thinking of :)

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 15, 2019

@GuillaumeGomez I will merge this if you not against it.

I'd really prefer that so that I can continue working on things. It seems like the closures PR needs some more time so I wouldn't like to block on that one, especially considering that the PRs for all the other crates are not ready yet and there are no changes to the manual code done there yet (removing manual stuff).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Jan 15, 2019

Well, I guess it's fine. I just hope it won't break my user-callback PR. :'(

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 15, 2019

Well, I guess it's fine. I just hope it won't break my user-callback PR. :'(

From going through your PR right now, I don't think so. There are probably one or two merge conflicts but those should be simple to solve as we touch completely different aspects of the codebase

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Jan 15, 2019

This should be merged together with the corresponding glib PR btw, otherwise I can't send PRs for everything else

@EPashkin EPashkin merged commit 3766446 into gtk-rs:master Jan 15, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 15, 2019

Then lets merge

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.