Re: [PATCH 2/2] Make readline completers a little more useful

[Jose E. Marchesi]

Hi John.
    
    diff --git a/src/pk-repl.c b/src/pk-repl.c
    index a81e371..3883686 100644
    --- a/src/pk-repl.c
    +++ b/src/pk-repl.c
    @@ -35,6 +35,65 @@
     #include <signal.h>
     #include <unistd.h>
     
    +extern struct pk_cmd null_cmd;

Is this extern still necessary?

    +static char *
    +poke_completion_function (const char *x, int state)
    +{
    +  static int idx = 0;
    +  static struct pkl_ast_node_iter iter;
    +  pkl_env env = pkl_get_env (poke_compiler);
    +  if (state == 0)
    +    {
    +      pkl_env_iter_begin (env, &iter);
    +      idx = 0;
    +    }
    +  else
    +    {
    +      if (pkl_env_iter_end (env, &iter))
    +   idx++;
    +      else
    +   pkl_env_iter_next (env, &iter);
    +    }
    +
    +  size_t len = strlen (x);
    +  char *function_name;
    +  function_name = get_next_matching_cmd (env, &iter, x, len);
    +  if (function_name)
    +    return function_name;
    +
    +  function_name = dot_command_completer_next (&idx, x, len);
    +  if (function_name)
    +    return function_name;
    +
    +  return NULL;
    +}

Much better.  Now the poke_completion_function limits itself to use the
PKL env iterators.

    +/*  Return the name of the next command that are currently
    +    in context of ENV and match NAME,LEN.  ITER is an iterator
    +    into the set of matches.  Returns the name of the next
    +    command in the set, or NULL if there are no more.
    +    The returned value must be freed by the caller.  */
    +char *
    +get_next_matching_cmd (pkl_env env, struct pkl_ast_node_iter *iter,
    +                  const char *name, size_t len)

I would change the name of the function to:

1) be coherent with the rest of public functions, i.e. to use a pkl_env_
   prefix.

2) I would avoid talking about "commands" in the PKL context.  From the
   compiler perspective there are no "commands".  There are
   declarations.  But since the PKL environment iterators iterate over
   declarations (which are the entities composed the environment) I see
   little value of reiterate it in the function's name.

3) The function works on an iterator, so why not having it as part of
   the env iterator API?

So what about just calling it pkl_env_iter_match?