Hi Andy,
Andy Wingo <wi...@pobox.com> writes:
> I'm OK with this in principle, but we shouldagree on names before this
> goes in.
Indeed, I often have trouble coming up with good names. We talked about
it on IRC, and agreed on 'scm_c_bind_keyword_arguments',
'SCM_ALLOW_OTHER_KEYS' and 'SCM_ALLOW_NON_KEYWORD_ARGUMENTS', which are
certainly better than what I had.
>> + va_start (va, allow_other_keys);
>
> "flags", no?
Yes, I noticed that shortly after posting. Oops! :)
>> + for (;;)
>> + {
>> + kw = va_arg (va, SCM);
>> + if (SCM_UNBNDP (kw))
>> + {
>> + /* KW_OR_ARG is not in the list of expected keywords. */
>> + if (!allow_other_keys)
>> + scm_error (scm_keyword_argument_error,
>> + subr, "Unrecognized keyword",
>> + SCM_EOL, SCM_BOOL_F);
>> + break;
>> + }
>
> Don't we need to advance "tail" in the "allow_other_keys" case, to skip
> over the argument value? That is what the bind-kwargs VM op does.
'tail' is not used again. 'rest' is advanced further down in the code.
>> + /* The next argument is not a keyword, or is a singleton
>> + keyword at the end of REST. */
>> + if (!allow_rest)
>> + scm_error (scm_keyword_argument_error,
>> + subr, "Invalid keyword",
>> + SCM_EOL, SCM_BOOL_F);
>> +
>> + /* Advance REST. */
>> + rest = tail;
>
> I think the semantics of rest arguments with keywords is that the rest
> argument *includes* the keywords.
Agreed. 'rest' is not returned, but just used internally to iterate
over the list.
As you suggested, I switched to using an enum for the flags.
You gave your blessing on IRC for me to push this after these updates,
so I did so.
Thanks!
Mark
