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

Value usage improvements #238

Merged
merged 4 commits into from Sep 30, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Sep 29, 2017

Fixes #223 and #228

sdroege added some commits Sep 29, 2017

Use &ToValue for ObjectExt::emit()
This improves usability and makes it more consistent with
Closure::invoke().

Fixes #228
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

Maybe stack allocator "hack" moved to function that accept array, both storage and len?

Member

EPashkin commented Sep 29, 2017

Maybe stack allocator "hack" moved to function that accept array, both storage and len?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

and maybe even more generalized by adding function parameter

Member

EPashkin commented Sep 29, 2017

and maybe even more generalized by adding function parameter

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 29, 2017

Member

What would the signature of the function you think of look like?

Member

sdroege commented Sep 29, 2017

What would the signature of the function you think of look like?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

I thought about something like this, but this version always allocate vector in stack,
as I don't see way to fill uninitialized vector.

fn func<'a, T,U, V: Fn(&U)->T>(f:V, values: &[U], stack: &'a mut[T], heap: &'a mut Vec<T>) -> &'a[T] {
  if values.len() <= stack.len() {
      for (i, arg) in values.iter().enumerate() {
          stack[i] = f(arg);
      }
      &stack[0..values.len()]
  } else {
      heap.reserve(stack.len());
      for (i, arg) in values.iter().enumerate() {
          heap[i] = f(arg);
      }
      heap.as_slice()
  }
}
Member

EPashkin commented Sep 29, 2017

I thought about something like this, but this version always allocate vector in stack,
as I don't see way to fill uninitialized vector.

fn func<'a, T,U, V: Fn(&U)->T>(f:V, values: &[U], stack: &'a mut[T], heap: &'a mut Vec<T>) -> &'a[T] {
  if values.len() <= stack.len() {
      for (i, arg) in values.iter().enumerate() {
          stack[i] = f(arg);
      }
      &stack[0..values.len()]
  } else {
      heap.reserve(stack.len());
      for (i, arg) in values.iter().enumerate() {
          heap[i] = f(arg);
      }
      heap.as_slice()
  }
}
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 29, 2017

Member

Note that this wouldn't cover both variants. The one in object.rs puts the instance argument as first, additional element into the vector.

Member

sdroege commented Sep 29, 2017

Note that this wouldn't cover both variants. The one in object.rs puts the instance argument as first, additional element into the vector.

Show outdated Hide outdated src/object.rs
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

Yes, you right. values can be changed to ExactSizeIterator.

Member

EPashkin commented Sep 29, 2017

Yes, you right. values can be changed to ExactSizeIterator.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

.. and second version use std::iter::once and chain.

Member

EPashkin commented Sep 29, 2017

.. and second version use std::iter::once and chain.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 29, 2017

Member

Maybe generalizing not worth memory or speed-wise, so I don't insist.
Also you can try https://crates.io/crates/smallvec as it seems doing almost same.

Member

EPashkin commented Sep 29, 2017

Maybe generalizing not worth memory or speed-wise, so I don't insist.
Also you can try https://crates.io/crates/smallvec as it seems doing almost same.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Sep 29, 2017

Member

It seemed like too much to depend on an external crate just for 4 lines of code :) but for refactoring the right solution would be a data type and not a function

Thanks for your other comments, I'll clean it up tomorrow

Member

sdroege commented Sep 29, 2017

It seemed like too much to depend on an external crate just for 4 lines of code :) but for refactoring the right solution would be a data type and not a function

Thanks for your other comments, I'll clean it up tomorrow

Use a stack allocated array instead of a slice for ObjectExt::emit() …
…/ Closure::invoke()

... if the number of arguments is less than 10.

Fixes #223
&s_args[0..args.len()+1]
} else {
v_args = Vec::with_capacity(args.len() + 1);
for arg in iter::once(&(self as &ToValue)).chain(args) {

This comment has been minimized.

@sdroege

sdroege Sep 29, 2017

Member

I'm not sure how to get rid of the &(self as &ToValue) disaster here. Any suggestions?

@sdroege

sdroege Sep 29, 2017

Member

I'm not sure how to get rid of the &(self as &ToValue) disaster here. Any suggestions?

This comment has been minimized.

@EPashkin

EPashkin Sep 29, 2017

Member

Why it disaster?
By way, I proposed use iter::once for function, if used directly then old variant looks normal (after index fix and with_capacity on second branch)

@EPashkin

EPashkin Sep 29, 2017

Member

Why it disaster?
By way, I proposed use iter::once for function, if used directly then old variant looks normal (after index fix and with_capacity on second branch)

This comment has been minimized.

@sdroege

sdroege Sep 30, 2017

Member

It looks ugly but works :) do you prefer it like this or the previous version?

@sdroege

sdroege Sep 30, 2017

Member

It looks ugly but works :) do you prefer it like this or the previous version?

This comment has been minimized.

@EPashkin

EPashkin Sep 30, 2017

Member

IMHO almost all same as long as it works

@EPashkin

EPashkin Sep 30, 2017

Member

IMHO almost all same as long as it works

This comment has been minimized.

@sdroege

sdroege Sep 30, 2017

Member

👍 Let's keep it as now then

@sdroege

sdroege Sep 30, 2017

Member

👍 Let's keep it as now then

Mark Value as #[repr(C)]
This way it's guaranteed to have the same representation as the
underlying gobject_ffi::GValue and can be safely transmuted/casted
between those two types (as is done in various places in GLib and
elsewhere).
@@ -93,6 +93,7 @@ use gobject_ffi;
/// (e.g. numeric types) don't.
///
/// See the [module documentation](index.html) for more details.
#[repr(C)]
pub struct Value(gobject_ffi::GValue);

This comment has been minimized.

@sdroege

sdroege Sep 30, 2017

Member

Let's get this in too then, this is not a new requirement for this PR but already before Closure and other places casted between glib::Value and gobject_ffi::GValue for preventing copies.

@sdroege

sdroege Sep 30, 2017

Member

Let's get this in too then, this is not a new requirement for this PR but already before Closure and other places casted between glib::Value and gobject_ffi::GValue for preventing copies.

This comment has been minimized.

@EPashkin

EPashkin Sep 30, 2017

Member

IMHO better be repr(internal) but now I can't find it at all.

@EPashkin

EPashkin Sep 30, 2017

Member

IMHO better be repr(internal) but now I can't find it at all.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Sep 30, 2017

Member

@sdroege Thanks
👍 for merge.

Member

EPashkin commented Sep 30, 2017

@sdroege Thanks
👍 for merge.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Sep 30, 2017

Member

Let's merge then. Thanks to both of you!

Member

GuillaumeGomez commented Sep 30, 2017

Let's merge then. Thanks to both of you!

@GuillaumeGomez GuillaumeGomez merged commit f4459da into gtk-rs:master Sep 30, 2017

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