Re: [RFA v2 22/24] Introduce gdb_argv, a class wrapper for buildargv

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-07-25 19:21, Tom Tromey wrote:
> @@ -254,11 +253,10 @@ gdbscm_string_to_argv (SCM string_scm)
>        return SCM_EOL;
>      }
> 
> -  c_argv = gdb_buildargv (string);
> +  gdb_argv c_argv (string);
>    for (i = 0; c_argv[i] != NULL; ++i)

Range-based for loop?

> diff --git a/gdb/procfs.c b/gdb/procfs.c
> index a0d2270..34e1996 100644
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -5113,11 +5113,8 @@ procfs_info_proc (struct target_ops *ops, const
> char *args,
>      }
> 
>    old_chain = make_cleanup (null_cleanup, 0);
> -  if (args)
> -    {
> -      argv = gdb_buildargv (args);
> -      make_cleanup_freeargv (argv);
> -    }
> +  gdb_argv built_argv (args);
> +  argv = built_argv.get ();
>    while (argv != NULL && *argv != NULL)

An opportunity to use a range based for ?

> @@ -890,7 +888,7 @@ pipe_windows_open (struct serial *scb, const char 
> *name)
>      const char *err_msg
>        = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | 
> PEX_BINARY_OUTPUT
>  		 | PEX_STDERR_TO_PIPE,
> -                 argv[0], argv, NULL, NULL,
> +                 argv[0], argv.get (), NULL, NULL,
>                   &err);
> 
>      if (err_msg)
> @@ -920,6 +918,7 @@ pipe_windows_open (struct serial *scb, const char 
> *name)
> 
>    scb->state = (void *) ps;
> 
> +  argv.release ();

This .release() seems to match previous behavior, but that previous 
behavior looks fishy to me.  Nothing seems to take ownership of that 
argv.  The only candidate would be pex_run, but as far as I can see it 
doesn't.

> diff --git a/gdb/stack.c b/gdb/stack.c
> index 7f8a51c..9c6d87e 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1872,13 +1872,14 @@ backtrace_command (char *arg, int from_tty)
>    int fulltrace_arg = -1, arglen = 0, argc = 0, no_filters  = -1;
>    int user_arg = 0;
> 
> +  gdb_argv built_argv;

Can this go in the "if" scope?

> diff --git a/gdb/utils.c b/gdb/utils.c
> index 06f4168..8a7a60f 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -2863,11 +2863,25 @@ ldirname (const char *filename)
>    return dirname;
>  }
> 
> +/* See utils.h.  */
> +
> +void
> +gdb_argv::reset (const char *s)
> +{
> +  char **argv = buildargv (s);
> +
> +  if (s != NULL && argv == NULL)
> +    malloc_failure (0);
> +
> +  freeargv (m_argv);
> +  m_argv = argv;
> +}
> +
>  /* Call libiberty's buildargv, and return the result.
>     If buildargv fails due to out-of-memory, call nomem.
>     Therefore, the returned value is guaranteed to be non-NULL,
>     unless the parameter itself is NULL.  */
> -
> +

Spurious space added here.

The patch LGTM (the .release() in the mingw code can be check later).  
You can address the other comments at your discretion before pushing.

Thanks,

Simon