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 support for getting a &str from a GValue without copying #162

Merged
merged 3 commits into from May 3, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented May 2, 2017

Needs some discussion if the lifetime parameter is good or not here. For G_TYPE_STRING the copying or not might not be that important, but I'll also have to implement this for more complex types in the future where copying is expensive.

Without the added lifetime parameter, the following would compile just fine (while it shouldn't):

    let s = {
        let v = Value::from("test");
        assert_eq!(v.get::<&str>(), Some("test"));
        v.get::<&str>()
    };

    assert_eq!(s, Some("test"));

(Note that using get::() instead above still works, as it should)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 2, 2017

Member

If this is accepted, I would also add something that can convert from a TypedValue<T: ToOwned> to a TypedValue<T::Owned> and back to allow easy switching between e.g. TypedValue and TypedValue<&str>.

Member

sdroege commented May 2, 2017

If this is accepted, I would also add something that can convert from a TypedValue<T: ToOwned> to a TypedValue<T::Owned> and back to allow easy switching between e.g. TypedValue and TypedValue<&str>.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 2, 2017

Member

@GuillaumeGomez wrote on IRC that this seems ok to him. So next step: 1) fixing all implementations so the tests succeed, 2) implement something to convert between e.g. TypedValue and TypedValue<&str> easily.

Any suggestions for the API for 2)? Or maybe just always work with TypedValue but have a get_ref() on it if FromValueOptional is implemented for the types involved, and one implements Borrow for the other?

Member

sdroege commented May 2, 2017

@GuillaumeGomez wrote on IRC that this seems ok to him. So next step: 1) fixing all implementations so the tests succeed, 2) implement something to convert between e.g. TypedValue and TypedValue<&str> easily.

Any suggestions for the API for 2)? Or maybe just always work with TypedValue but have a get_ref() on it if FromValueOptional is implemented for the types involved, and one implements Borrow for the other?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 2, 2017

Member

For first part only gdk need fix.
We don't have other implementation currently. and we only need it in gdk as I unable to define implementations for some BoxedValue parallel with IsA<Object> implementations
👍 for merge this ignoring build error.

Member

EPashkin commented May 2, 2017

For first part only gdk need fix.
We don't have other implementation currently. and we only need it in gdk as I unable to define implementations for some BoxedValue parallel with IsA<Object> implementations
👍 for merge this ignoring build error.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 3, 2017

Member

For 2) I don't see any useful way of doing that generically, because ToOwned/Borrowed work with "str" as type instead of "&str". We could define our own mapping traits here, which seems like quite some overhead, or just implement From<_> for the conversion between TypedValue<&str> and TypedValue in both directions.

@GuillaumeGomez what do you think? The pull request is now updated with From<_> impls.

Member

sdroege commented May 3, 2017

For 2) I don't see any useful way of doing that generically, because ToOwned/Borrowed work with "str" as type instead of "&str". We could define our own mapping traits here, which seems like quite some overhead, or just implement From<_> for the conversion between TypedValue<&str> and TypedValue in both directions.

@GuillaumeGomez what do you think? The pull request is now updated with From<_> impls.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

Just a small nit/suggestion but otherwise it's all good.

Member

GuillaumeGomez commented May 3, 2017

Just a small nit/suggestion but otherwise it's all good.

sdroege added some commits May 2, 2017

Add lifetime parameter to FromValueOptional and FromValue
This allows properly implementing zero-copy getters for Values, e.g. to
get a &str while making sure its lifetime is bound to the value's.
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

@EPashkin: Ok for me, tell me when it's ok for you as well so I can merge.

Member

GuillaumeGomez commented May 3, 2017

@EPashkin: Ok for me, tell me when it's ok for you as well so I can merge.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

@GuillaumeGomez yes, IMHO in can be merged

Member

EPashkin commented May 3, 2017

@GuillaumeGomez yes, IMHO in can be merged

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

Great! Thanks!

Member

GuillaumeGomez commented May 3, 2017

Great! Thanks!

@GuillaumeGomez GuillaumeGomez merged commit d40b834 into gtk-rs:master May 3, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment