Re: [PATCH] Implement 'scm_c_bind_kwargs' to handle keyword arguments from C

[Andy Wingo <wingo@pobox.com>]

Hi Mark,

I'm OK with this in principle, but we shouldagree on names before this
goes in.

On Sat 06 Apr 2013 21:31, Mark H Weaver <mhw@netris.org> writes:

> * libguile/keywords.c (scm_keyword_argument_error): New variable.
>   (scm_c_bind_kwargs): New API function.

I think I prefer scm_c_bind_keyword_arguments, if that's OK with you.

> +scm_c_bind_kwargs (const char *subr, SCM rest, int flags, ...)
> +{
> +  int allow_other_keys = flags & SCM_KWARGS_ALLOW_OTHER_KEYS;
> +  int allow_rest       = flags & SCM_KWARGS_ALLOW_REST;

"Allow rest" sorta sounds like the #:rest bit from lambda*, but it
doesn't actually help us to bind a rest argument, because we already
have it.  Better to call it "ALLOW_NON_KEYWORDS" or "PERMISSIVE" or
"SLOPPY" or something.

> +  if (SCM_UNLIKELY (!allow_rest && scm_ilength (rest) % 2 != 0))
> +    scm_error (scm_keyword_argument_error,
> +               subr, "Odd length of keyword argument list",
> +               SCM_EOL, SCM_BOOL_F);
> +
> +  while (scm_is_pair (rest))
> +    {
> +      SCM kw_or_arg = SCM_CAR (rest);
> +      SCM tail = SCM_CDR (rest);
> +
> +      if (scm_is_keyword (kw_or_arg) && scm_is_pair (tail))
> +        {
> +          SCM kw;
> +          SCM *arg_p;
> +
> +          va_start (va, allow_other_keys);

"flags", no?

> +          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.

> +          /* 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.

> +#define SCM_KWARGS_ALLOW_OTHER_KEYS  1
> +#define SCM_KWARGS_ALLOW_REST        2

typedef enum
{
  SCM_KEYWORD_ARGUMENTS_NO_FLAGS = 0,
  SCM_KEYWORD_ARGUMENTS_ALLOW_OTHER_KEYS = (1U << 0),
  SCM_KEYWORD_ARGUMENTS_IGNORE_NON_KEYWORDS = (1U << 1)
} scm_t_keyword_arguments_flag;

Cheers,

Andy
-- 
http://wingolog.org/