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 generic pointer array impls of ToGlibPtr and FromGlibContainer #198

Merged
merged 3 commits into from Jul 18, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jul 14, 2017

This introduces new traits:

  • ToGlibContainerFromSlice: Conversion from a &[Self] into some GLib
    container (array, GList, etc).

  • FromGlibContainerAsVec: Conversion from a GLib container (array,
    GList, etc) to a Vec. This always requires the length to be
    given and can be used for non-pointer containers (e.g. uint8_t[]).

  • FromGlibPtrContainerAsVec: Some as FromGlibContainerAsVec but does not
    require the length, and instead requires pointers as elements and the
    container to be NULL-terminated

They are all implemented for T and not [T]/Vec, which is required for this to
be possible to implement in crates other than the glib crate.

impls for Strings and fundamental types exist in GLib, glib_wrapper!()
is automatically implementing them too.

Based on these there are generic implementations for ToGlibPtr for [T]
and FromGlibContainer for Vec.

Also split FromGlibPtrContainer into FromGlibContainer and FromGlibPtrContainer,
the first requiring a length to be given (and thus is usable for
non-NULL terminated or non-pointer containers), the second requires
pointers and NULL-termination.

With all this in place it is now possible to implement conversions
from/to containers of fundamental types, enums, flags, etc.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 14, 2017

Member

I already said it earlier but for the records: didn't see anything shocking. Thanks for the huge update!

Member

GuillaumeGomez commented Jul 14, 2017

I already said it earlier but for the records: didn't see anything shocking. Thanks for the huge update!

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 14, 2017

Member

It might make sense to generate the impls in the macros by yet another macro, they're basically copy&paste right now. But that would mean that GLib exports yet another macro, so I'm not sure about that. Your decision

Member

sdroege commented Jul 14, 2017

It might make sense to generate the impls in the macros by yet another macro, they're basically copy&paste right now. But that would mean that GLib exports yet another macro, so I'm not sure about that. Your decision

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 14, 2017

Member

It depends on the final outcome: if we still just have to call one macro (which will call whatever macros), it's fine. I'd just prefer that we don't have to directly call multiple macros. (If that makes sense?)

Member

GuillaumeGomez commented Jul 14, 2017

It depends on the final outcome: if we still just have to call one macro (which will call whatever macros), it's fine. I'd just prefer that we don't have to directly call multiple macros. (If that makes sense?)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 14, 2017

Member

Hm, the new traits should probably have different function names. Otherwise they conflict with the other ones and have to be explicitly specified. Doing so later, but for an initial review this should be ok.

Member

sdroege commented Jul 14, 2017

Hm, the new traits should probably have different function names. Otherwise they conflict with the other ones and have to be explicitly specified. Doing so later, but for an initial review this should be ok.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 14, 2017

Member

Looks good for fast check, will check more tomorrow.
Thanks, @sdroege

Member

EPashkin commented Jul 14, 2017

Looks good for fast check, will check more tomorrow.
Thanks, @sdroege

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 14, 2017

Member

It depends on the final outcome: if we still just have to call one macro (which will call whatever macros), it's fine. I'd just prefer that we don't have to directly call multiple macros. (If that makes sense?)

Yes it would still just be glib_wrapper!() that is called... but internally it would call some new macro

Member

sdroege commented Jul 14, 2017

It depends on the final outcome: if we still just have to call one macro (which will call whatever macros), it's fine. I'd just prefer that we don't have to directly call multiple macros. (If that makes sense?)

Yes it would still just be glib_wrapper!() that is called... but internally it would call some new macro

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 14, 2017

Member

Actually can't use macros for this because of different mutability, and that's not easy to abstract over

Member

sdroege commented Jul 14, 2017

Actually can't use macros for this because of different mutability, and that's not easy to abstract over

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 15, 2017

Member

Cairo now need adding feature like "use_glib" instead "glib" to include both "glib" and "glib_sys" crates

Member

EPashkin commented Jul 15, 2017

Cairo now need adding feature like "use_glib" instead "glib" to include both "glib" and "glib_sys" crates

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 18, 2017

Member

All complete and compiling from my side now

Member

sdroege commented Jul 18, 2017

All complete and compiling from my side now

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 18, 2017

Member

I'd prefer for this (and the related PRs) to be merged before my other PRs btw. I'll rebase the others as needed then (they just add features, don't break things).

Member

sdroege commented Jul 18, 2017

I'd prefer for this (and the related PRs) to be merged before my other PRs btw. I'll rebase the others as needed then (they just add features, don't break things).

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 18, 2017

Member

Thanks, @sdroege all PRs look good, only last update to last gir when needed.

Member

EPashkin commented Jul 18, 2017

Thanks, @sdroege all PRs look good, only last update to last gir when needed.

sdroege added some commits Jul 14, 2017

Remove generic pointer array impls of ToGlibPtr and FromGlibContainer
This introduces new traits:
- ToGlibContainerFromSlice: Conversion from a &[Self] into some GLib
  container (array, GList, etc).

- FromGlibContainerAsVec: Conversion from a GLib container (array,
  GList, etc) to a Vec<Self>. This always requires the length to be
  given and can be used for non-pointer containers (e.g. uint8_t[]).

- FromGlibPtrContainerAsVec: Some as FromGlibContainerAsVec but does not
  require the length, and instead requires pointers as elements and the
  container to be NULL-terminated

They are all implemented for T and not [T]/Vec<T>, which is required for this to
be possible to implement in crates other than the glib crate.

impls for Strings and fundamental types exist in GLib, glib_wrapper!()
is automatically implementing them too.

Based on these there are generic implementations for ToGlibPtr for [T]
and FromGlibContainer for Vec<T>.

Also split FromGlibPtrContainer into FromGlibContainer and FromGlibPtrContainer,
the first requiring a length to be given (and thus is usable for
non-NULL terminated or non-pointer containers), the second requires
pointers and NULL-termination.

With all this in place it is now possible to implement conversions
from/to containers of fundamental types, enums, flags, etc.
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 18, 2017

Member

Updated gir

Member

sdroege commented Jul 18, 2017

Updated gir

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 18, 2017

Member

@GuillaumeGomez, 👍 for merge this and all linked PRs

Member

EPashkin commented Jul 18, 2017

@GuillaumeGomez, 👍 for merge this and all linked PRs

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 18, 2017

Member

Taking a look in a few hours.

Member

GuillaumeGomez commented Jul 18, 2017

Taking a look in a few hours.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 18, 2017

Member

Thanks a lot!

Member

GuillaumeGomez commented Jul 18, 2017

Thanks a lot!

@GuillaumeGomez GuillaumeGomez merged commit afa7242 into gtk-rs:master Jul 18, 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
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 18, 2017

Member

Thanks :) now only gio and gdk-pixbuf are missing

Member

sdroege commented Jul 18, 2017

Thanks :) now only gio and gdk-pixbuf are missing

@EPashkin EPashkin referenced this pull request Dec 3, 2017

Closed

Memory leak #267

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