Allow Enumerator size argument to be any callable, not just a Proc or Integer. #362

Closed
wants to merge 4 commits into
from

6 participants

@avdi
avdi commented Jul 17, 2013

Addresses [Bug #8641]

This is my very first attempt at hacking Ruby C code, and is TOTALLY cargo-culted. I don't expect this to be merged in, but I would LOVE some feedback so I can get better at this. Thank you!

@avdi
avdi commented Jul 17, 2013

I think some screwy whitespace might have made it's way in. Sorry, I need to set up Emacs for Ruby conventions.

@marcandre
Member

In addition to the whitespace, ideally the test would be more strict by using an Object with a singleton method #call instead of a Method.

Thanks for helping with the patch, it's appreciated especially that I'm travelling right now.

@avdi
avdi commented Jul 17, 2013

Duh. I think I meant to write it that way in the first place and then got all stupid about it. Just pushed a better test.

@avdi
avdi commented Jul 17, 2013

BTW I'm kinda confused why e->args would ever be used in calling the "size" callable. I just copied that from the proc version of the code but it seems odd to me.

@charliesome charliesome and 2 others commented on an outdated diff Jul 17, 2013
if (e->size_fn) {
return (*e->size_fn)(e->obj, e->args, obj);
}
- if (rb_obj_is_proc(e->size)) {
- if (e->args)
- return rb_proc_call(e->size, e->args);
- else
- return rb_proc_call_with_block(e->size, 0, 0, Qnil);
+ if (rb_respond_to(e->size, id_call)) {
+ if (e->args) {
+ argc = RARRAY_LENINT(e->args);
+ argv = RARRAY_PTR(e->args);
@charliesome
charliesome Jul 17, 2013 Member

You should prefer to declare these variables here, rather than at the top of the function so it's easier to see what types they are

@avdi
avdi Jul 17, 2013

Last I checked that's not allowed in C. What version of C is this expected to work in?

@charliesome
charliesome Jul 17, 2013 Member

You can declare variables at the top of any block, not just functions.

@avdi
avdi Jul 17, 2013

Hm, I'm rustier on C than I thought. Will push up a change.

@charliesome
charliesome Jul 17, 2013 Member

I'm not sure if this is actually allowed in C, or whether this is an extension compilers generally tend to implement, but there's other code in Ruby that declares variables at the top of non-function blocks so it's safe to do this.

@henrikhodne
henrikhodne Jul 17, 2013

This isn't allowed in C89, but is allowed in C99 as far as I know.

@avdi
avdi commented Jul 17, 2013

Specifically, I'm trying to think of a case where the args to be passed to the method to enumerate over would be the same as the args to be passed to the size callable.

@avdi
avdi commented Jul 17, 2013

BTW is it protocol for this to include a Changelog update?

@zzak
Member
zzak commented Jul 17, 2013

@avdi Usually the person who commits your patch will write the ChangeLog

However, NEWS file changes are welcome

@Hanmac
Hanmac commented Jul 17, 2013

hm i dont know but would it make a difference in benchmark if we test for proc first and then use rb_proc_call and if its not a proc than use the rb_respond_to? i think that rb_proc_call may be faster than rb_funcall2 ?

@marcandre
Member

Thanks for the changes, looks like the patch is ok now. I should commit it soon.

As for the arguments question, it was either never pass any, or pass them along which allowed something like:

class Foo
    def bar(*args)
      return to_enum(:bar, &method(:bar_size)) unless block_given?
      # yield according to args
    end
    def bar_size(*args)
      # calculate according to args
    end
end

Of course it would have been possible to use a block like to_enum(:bar) { bar_size(*args) } which makes the argument passing not necessary.

@avdi
avdi commented Jul 19, 2013

Thanks for the explanation. When I thought it through I came up with
something similar, but I couldn't think of a real-world example. It's not
that usual in Ruby to have multiple methods which all take the same
arguments, simply because whenever we see that we tend to turn it into a
class instead. So I'm still curious if there are any real-world examples.

Anyway, not a huge deal. I'm just excited that it may make it in!

On Thu, Jul 18, 2013 at 7:29 PM, Marc-André Lafortune <
notifications@github.com> wrote:

Thanks for the changes, looks like the patch is ok now. I should commit it
soon.

As for the arguments question, it was either never pass any, or pass them
along which allowed something like:

class Foo
def bar(_args)
return to_enum(:bar, &method(:bar_size)) unless block_given?
# yield according to args
end
def bar_size(_args)
# calculate according to args
end
end

Of course it would have been possible to use a block like to_enum(:bar) {
bar_size(*args) } which makes the argument passing not necessary.


Reply to this email directly or view it on GitHubhttps://github.com/ruby/ruby/pull/362#issuecomment-21222461
.

Avdi Grimm
http://avdi.org

I only check email twice a day. to reach me sooner, go to
http://awayfind.com/avdi

@avdi
avdi commented Jul 19, 2013

On Wed, Jul 17, 2013 at 2:22 PM, Hanmac notifications@github.com wrote:

hm i dont know but would it make a difference in benchmark if we test for
proc first and then use rb_proc_call and if its not a proc than use the
rb_respond_to? i think that rb_proc_call may be faster than rb_funcall2 ?

The performance difference sounds plausible, but I'm curious if the Ruby
libs do this optimization anywhere else where they rely on a callable? If
so I'd love to see it and learn from it.

@marcandre marcandre added a commit that closed this pull request Aug 27, 2013
@marcandre marcandre * enumerator.c: Allow Enumerator size argument to be any callable.
  Patch by Avdi Grimm. [bug #8641] [ruby-core:56032] [fix GH-362]

* test/ruby/test_enumerator.rb: Test for above

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42698 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
ca7f52a
@marcandre marcandre closed this in ca7f52a Aug 27, 2013
@marcandre
Member

Sorry for the delay, the PR escaped my mind while I was travelling...

@tenderlove tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014
@marcandre marcandre * enumerator.c: Allow Enumerator size argument to be any callable.
  Patch by Avdi Grimm. [bug #8641] [ruby-core:56032] [fix GH-362]

* test/ruby/test_enumerator.rb: Test for above

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@42698 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
3751562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment