Re: [PATCH 3/3] Build command tries recusively

[Oliver O'Halloran]

On Tue, Oct 1, 2019 at 11:18 PM Jose E. Marchesi <address@hidden> wrote:
>
>
>     We can use the presence or absence of the subcommand list to determine if
>     the subtrie needs to be initialised rather than open-coding it. This makes
>     the command init process a bit simpler and self-synchronsing.
>
>     Signed-off-by: Oliver O'Halloran <address@hidden>
>     ---
>      src/pk-cmd.c  | 43 +++++++++++++++++--------------------------
>      src/pk-cmd.h  |  2 +-
>      src/pk-help.c |  4 +---
>      src/pk-info.c |  4 +---
>      src/pk-set.c  |  4 +---
>      src/pk-vm.c   |  8 ++------
>      6 files changed, 23 insertions(+), 42 deletions(-)
>
>     diff --git a/src/pk-cmd.c b/src/pk-cmd.c
>     index 7500471..4a53bb8 100644
>     --- a/src/pk-cmd.c
>     +++ b/src/pk-cmd.c
>     @@ -199,8 +199,13 @@ pk_trie_from_cmds (struct pk_cmd *cmds[])
>            /* Note this assumes no commands with empty names.  */
>            t->cmd = cmd;
>            t = root;
>     +
>     +      /* if we have subcommands, expand their trie too */
>     +      if (cmd->subcmds)
>     +        cmd->subtrie = pk_trie_from_cmds(cmd->subcmds);
>
> Please leave a space between the function name and the argument list.

Sorry, force of habit ;)

For Linux we have checkpatch.pl which is a terrible pile of hacks, but
it does catch most of the common style violations. Is there anything
similar for projects using the GNU standards? I tried using GNU indent
since that (allegedly) formats to GNU style by default, but:

[23:45 oliver ~/.../poke/src ((0f5563d...) %)]$ indent *.[ch]
indent: pkl-pass.c:396: Error:Unmatched 'else'

I'm not sure what it's complaining about since pkl-pass.c:396 is a
break statement. and:

[23:46 oliver ~/.../poke/src ((0f5563d...) *%)]$ git diff --shortstat
 41 files changed, 6021 insertions(+), 6141 deletions(-)

So.. I guess GNU indent is not the best thing for the job?

>
>         }
>
>     +  /* associate the command with all the intermediary trie nodes */
>        pk_trie_expand_cmds (root, root);
>        return root;
>      }
>     @@ -312,7 +317,7 @@ pk_cmd_exec_1 (char *str, struct pk_trie *cmds_trie, 
> char *prefix)
>              goto usage;
>
>            if (*p != '\0')
>     -        return pk_cmd_exec_1 (p, *cmd->subtrie, cmd_name);
>     +        return pk_cmd_exec_1 (p, cmd->subtrie, cmd_name);
>          }
>
>        /* Parse arguments.  */
>     @@ -585,21 +590,6 @@ pk_cmd_exec_1 (char *str, struct pk_trie *cmds_trie, 
> char *prefix)
>        return 0;
>      }
>
>     -extern struct pk_cmd *info_cmds[]; /* pk-info.c  */
>     -extern struct pk_trie *info_trie; /* pk-info.c  */
>     -
>     -extern struct pk_cmd *help_cmds[]; /* pk-help.c */
>     -extern struct pk_trie *help_trie; /* pk-help.c */
>     -
>     -extern struct pk_cmd *vm_cmds[]; /* pk-vm.c  */
>     -extern struct pk_trie *vm_trie;  /* pk-vm.c  */
>     -
>     -extern struct pk_cmd *vm_disas_cmds[];  /* pk-vm.c */
>     -extern struct pk_trie *vm_disas_trie; /* pk-vm.c */
>     -
>     -extern struct pk_cmd *set_cmds[]; /* pk-set.c */
>     -extern struct pk_trie *set_trie; /* pk-set.c */
>     -
>      static struct pk_trie *cmds_trie;
>
>      int
>     @@ -747,11 +737,6 @@ void
>      pk_cmd_init (void)
>      {
>        cmds_trie = pk_trie_from_cmds (cmds);
>     -  info_trie = pk_trie_from_cmds (info_cmds);
>     -  help_trie = pk_trie_from_cmds (help_cmds);
>     -  vm_trie = pk_trie_from_cmds (vm_cmds);
>     -  vm_disas_trie = pk_trie_from_cmds (vm_disas_cmds);
>     -  set_trie = pk_trie_from_cmds (set_cmds);
>
>        /* Compile commands written in Poke.  */
>        {
>     @@ -779,10 +764,16 @@ pk_cmd_init (void)
>      void
>      pk_cmd_shutdown (void)
>      {
>     +  struct pk_cmd *cmd;
>     +  int i;
>     +
>     +  for (i = 0, cmd = cmds[0];
>     +       cmd->name != NULL;
>     +       cmd = cmds[++i])
>     +    {
>     +      if (cmd->subtrie)
>     +        pk_trie_free (cmd->subtrie);
>     +    }
>     +
>        pk_trie_free (cmds_trie);
>     -  pk_trie_free (info_trie);
>     -  pk_trie_free (help_trie);
>     -  pk_trie_free (vm_trie);
>     -  pk_trie_free (vm_disas_trie);
>     -  pk_trie_free (set_trie);
>      }
>
> Hm, but this won't work recursively right?  i.e. what about
> vm_disas_trie.

It should. The vm_disas_cmd has vm_disas_cmds[] as it's subcommand
list and the change in pk_trie_from_cmds() willl run recursively if
the subcommand list is non-null.

i.e.

(poke) .vm disassemble
Usage: vm disassemble (expression|function)
subcommands:
      expression - vm disassemble expression[/n] EXP
Flags:
  n (do a native disassemble)
      function - vm disassemble function[/n] FUNCTION_NAME
Flags:
  n (do a native disassemble)
      mapper - vm disassemble mapper[/n] EXPRESSION
Flags:
  n (do a native disassemble)
      writter - vm disassemble writer[/n] EXPRESSION
Flags:
  n (do a native disassemble)
(poke)

The output is a little weird due to the usage strings. Removing them:

(poke) .vm disassemble
Usage: vm disassemble (expression|function)
subcommands:
      expression - vm disassemble expression[/n] EXP
      function - vm disassemble function[/n] FUNCTION_NAME
      mapper - vm disassemble mapper[/n] EXPRESSION
      writter - vm disassemble writer[/n] EXPRESSION
(poke)

Which I think is what you want?