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

Implement FromGlibPtrNone and FromGlibPtrFull for Value #172

Merged
merged 3 commits into from May 24, 2017

Conversation

Projects
None yet
4 participants
@sdroege
Member

sdroege commented May 21, 2017

There are some functions that need that in GStreamer.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 21, 2017

Member

Maybe better add 2 other options but IMHO it can wait (don't like add unused functions)
@GuillaumeGomez 👍 for this

Member

EPashkin commented May 21, 2017

Maybe better add 2 other options but IMHO it can wait (don't like add unused functions)
@GuillaumeGomez 👍 for this

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 21, 2017

Member

I can also add the other trait impls, might be good for consistency

Member

sdroege commented May 21, 2017

I can also add the other trait impls, might be good for consistency

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 22, 2017

Member

So should I implement impls for the other traits (which?) or we just wait until someone needs it? :)

Member

sdroege commented May 22, 2017

So should I implement impls for the other traits (which?) or we just wait until someone needs it? :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 22, 2017

Member

IMHO it good as is.

Member

EPashkin commented May 22, 2017

IMHO it good as is.

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 23, 2017

Member

I also implemented these traits in my pull request.
I'm not sure which way is better but mine doesn't involve a copy (not sure if it is correct, thought).

By the way, are some implementions missing in this pull request?
I've got 4 implementations in mine.

Member

antoyo commented May 23, 2017

I also implemented these traits in my pull request.
I'm not sure which way is better but mine doesn't involve a copy (not sure if it is correct, thought).

By the way, are some implementions missing in this pull request?
I've got 4 implementations in mine.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 23, 2017

Member

@antoyo Sorry, I should've checked before. Didn't expect another pull request to do this as it's rather uncommon for C code to give you GValue pointers.

See my review comments on your code. I could add the impl FromGlibPtrNone<*mut gobject_ffi::GValue> for Value as that one is technically correct, but the impl FromGlibPtrFull<*const gobject_ffi::GValue> for Value is not. Anyway, see comments there :)

Should we finish your pull request and get that one merged because you were first, or just merge this one?

Member

sdroege commented May 23, 2017

@antoyo Sorry, I should've checked before. Didn't expect another pull request to do this as it's rather uncommon for C code to give you GValue pointers.

See my review comments on your code. I could add the impl FromGlibPtrNone<*mut gobject_ffi::GValue> for Value as that one is technically correct, but the impl FromGlibPtrFull<*const gobject_ffi::GValue> for Value is not. Anyway, see comments there :)

Should we finish your pull request and get that one merged because you were first, or just merge this one?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 23, 2017

Member

I added impl FromGlibPtrNone<*mut gobject_ffi::GValue> for Value too here. I guess let's get in my pull request for now, and @antoyo 's then only for the GClosure stuff (thanks btw, I will also need that soonish)

Member

sdroege commented May 23, 2017

I added impl FromGlibPtrNone<*mut gobject_ffi::GValue> for Value too here. I guess let's get in my pull request for now, and @antoyo 's then only for the GClosure stuff (thanks btw, I will also need that soonish)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 23, 2017

Member

@antoyo in your case you probably also want to prevent the copying to happen (you just want to borrow the values from the arguments to the marshal function), for which it would make sense to add a lifetime parameter to FromGlibPtrNone (and maybe Full too?), and then implement it for &'a Value too

@GuillaumeGomez @EPashkin Any opinions on adding a lifetime parameter to FromGlibPtr*, similar to what I did for the FromGValue some time ago?

Member

sdroege commented May 23, 2017

@antoyo in your case you probably also want to prevent the copying to happen (you just want to borrow the values from the arguments to the marshal function), for which it would make sense to add a lifetime parameter to FromGlibPtrNone (and maybe Full too?), and then implement it for &'a Value too

@GuillaumeGomez @EPashkin Any opinions on adding a lifetime parameter to FromGlibPtr*, similar to what I did for the FromGValue some time ago?

@antoyo

This comment has been minimized.

Show comment
Hide comment
@antoyo

antoyo May 23, 2017

Member

I don't have any problem on basing my pull request on this work (thanks for you work too ;) ).

I read about memory management in glib and I wonder if FromGlibPtrNone from glib is actually correct.
From what I understand, when there's no ownership transfer, it is similar to &T in Rust, so I believe from_glib_none should actually return a reference.
But I can see in the documentation that FromGlibPtrNone<*const c_char> for String is implemented and that copies the String.

I don't understand yet how all these traits work.
Do I need FromGlibPtrBorrow to get a reference to a value that I get with transfert none?
If yes, I would need the implementation of this trait in this pull request.

Member

antoyo commented May 23, 2017

I don't have any problem on basing my pull request on this work (thanks for you work too ;) ).

I read about memory management in glib and I wonder if FromGlibPtrNone from glib is actually correct.
From what I understand, when there's no ownership transfer, it is similar to &T in Rust, so I believe from_glib_none should actually return a reference.
But I can see in the documentation that FromGlibPtrNone<*const c_char> for String is implemented and that copies the String.

I don't understand yet how all these traits work.
Do I need FromGlibPtrBorrow to get a reference to a value that I get with transfert none?
If yes, I would need the implementation of this trait in this pull request.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 23, 2017

Member

Basically what we would need for borrowing is impl<'a> FromGlibPtrNone<*const gobject_ffi::GValue> for &'a Value for example. Currently all implementations of FromGlibPtrNone are doing a copy (or another ref()), which is also correct. "transfer none" in GLib just means that you, as the caller of that function, don't own the return value. So taking a copy is a valid thing to do in that case, while with "transfer full" we could just take the return value without copying (but often can't because of different allocators, etc).

But yes, it is basically like a &T, just that we currently always clone() it directly.

Member

sdroege commented May 23, 2017

Basically what we would need for borrowing is impl<'a> FromGlibPtrNone<*const gobject_ffi::GValue> for &'a Value for example. Currently all implementations of FromGlibPtrNone are doing a copy (or another ref()), which is also correct. "transfer none" in GLib just means that you, as the caller of that function, don't own the return value. So taking a copy is a valid thing to do in that case, while with "transfer full" we could just take the return value without copying (but often can't because of different allocators, etc).

But yes, it is basically like a &T, just that we currently always clone() it directly.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 23, 2017

Member

See also #162 for a similar case, but I think we should discuss this in a separate issue. It's unrelated to this very pull request, but something that would be good to have done on top of it (and sooner or later I might also need that too).

Member

sdroege commented May 23, 2017

See also #162 for a similar case, but I think we should discuss this in a separate issue. It's unrelated to this very pull request, but something that would be good to have done on top of it (and sooner or later I might also need that too).

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

@GuillaumeGomez Anything blocking this from being merged?

Member

sdroege commented May 24, 2017

@GuillaumeGomez Anything blocking this from being merged?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 24, 2017

Member

No, we're all good. Thanks!

Member

GuillaumeGomez commented May 24, 2017

No, we're all good. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 5e4e2b4 into gtk-rs:master May 24, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 24, 2017

Member

Great, thanks :)

Member

sdroege commented May 24, 2017

Great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment