From: Yusuke Endoh <mame@...>
Date: 2012-03-13T20:38:24+09:00
Subject: [ruby-core:43261] Re: [ruby-trunk - Bug #6085] Treatment of Wrong Number of Arguments

Hello,

> On Mon, Mar 12, 2012 at 10:31 AM, Yui NARUSE <naruse@airemix.jp> wrote:
>> Use one of follwing:
>> * https://github.com/marcandre/ruby/compare/rb_arity_check
>> * https://github.com/marcandre/ruby/compare/rb_arity_check.diff
>> * https://github.com/marcandre/ruby/compare/rb_arity_check.patch
>
> Nice, thanks! I'll provide this kind of link in the future, quite helpful.

Cool, thanks.


2012/3/13, Marc-Andre Lafortune <ruby-core@marc-andre.ca>:
> On Mon, Mar 12, 2012 at 8:07 AM, Yusuke Endoh <mame@tsg.ne.jp> wrote:
>> One concern: I'm afraid if this change affects people who parses
>> the message string of WNA.  What do you think?  There is not such
>> people, is there?  I don't want to be pedantic, but I can't feel
>> sure because I can no longer use Google codesearch...  Google!!
>
> The error type is part of the language specs, but I feel like error messages
> are not meant to be parsed and are subject to change. In this particular
> case, I just checked and Rubinius gives different error messages
> (ArgumentError: method 'upcase': given 1, expected 0).

Sounds good.  At least, Rubinius community does not know any actual
case where WNA message is parsed.


> The changes I propose
> are also minimal in their approach and make parsing even easier!

You know, making parsing easy is not the purpose or the right way.
My concern is just about compatiblity.


>> Anyway, I agree that the current is awkward.  If no one complains,
>> I'm positive to import it tentatively.
>
> Thanks. Just let me know after you've looked at it and I'll gladly commit
> these.

Looks good to me.
It brings not only behavior consistency but also good refactoring
effect.

I noticed some minor issues below.

vm_insnhelper.c:

    +static inline VALUE
    +rb_arg_error_new(int argc, int min, int max) {
    +    const char *template = "wrong number of arguments (%d for %d..%d)";
    +    if (min == max) {
    +	template = "wrong number of arguments (%d for %d)";
    +    }
    +    else if (max == UNLIMITED_ARGUMENTS) {
    +	template = "wrong number of arguments (%d for %d+)";
    +    }
    +    return rb_exc_new3(rb_eArgError, rb_sprintf(template, argc, min, max));
    +}

It would be good to match the number of %d and actual arguments.


eval.c:

    -    if (i < argc) goto wrong_args;
    +    if (i < argc) rb_raise(rb_eArgError, "wrong arguments");

I guess this line can be removed, though this is not your fault.


test/ruby/test_arity.rb

    assert_equal "0 for 1",     err_mess{ "".sub!{} }

This assertion fails.  Did you mean "0 for 1..2" ?


>> Off topic.  Are you interested in improving a keyword argument?
>> There is some issues on its implementation, but I have no time to
>> work on it :-(
>
> I'm not sure I have the technical skills needed, but I can definitely try to
> help. In any case I wanted to work on checking for named arguments and
> giving a better error message in those cases too. What else could I help on?

So far, the remaining issues I know are better error message, and #5989.

-- 
Yusuke Endoh <mame@tsg.ne.jp>