Re: [PATCH] Simplify macro_define_command

[Simon Marchi <simark@simark.ca>]

On 4/22/24 11:49 AM, Tom Tromey wrote:
> This simplifies macro_define_command, using std::vector to remove some
> manual memory management.  temporary_macro_definition is also
> simplified -- much of the code there was simply unnecessary.
> 
> Regression tested on x86-64 Fedora 38.
> ---
>  gdb/macrocmd.c | 55 +++++++++++++++-----------------------------------
>  1 file changed, 16 insertions(+), 39 deletions(-)
> 
> diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
> index ddabfede0f5..0d0db5d1b90 100644
> --- a/gdb/macrocmd.c
> +++ b/gdb/macrocmd.c
> @@ -308,33 +308,24 @@ extract_identifier (const char **expp, int is_parameter)
>    return gdb::unique_xmalloc_ptr<char> (result);
>  }
>  
> -struct temporary_macro_definition : public macro_definition
> +struct temporary_macro_definition
>  {
> -  temporary_macro_definition ()
> +  ~temporary_macro_definition ()
>    {
> -    table = nullptr;
> -    kind = macro_object_like;
> -    argc = 0;
> -    argv = nullptr;
> -    replacement = nullptr;
> +    free_vector_argv (argv);
>    }

Since this structure has a destructor, perhaps make it
non-copyable/assignable/etc (rule of 3/5)?

It could be a subsequent cleanup, but that class could have a more
general name and live in gdbsupport/common-utils.h.  It could then be
used by other current users of free_vector_argv.

> @@ -358,26 +343,18 @@ macro_define_command (const char *exp, int from_tty)
>  
>        while (*exp != ')')
>  	{
> -	  int i;
> -
> -	  if (new_macro.argc == alloced)
> -	    {
> -	      alloced *= 2;
> -	      argv = (char **) xrealloc (argv, alloced * sizeof (char *));
> -	      /* Must update new_macro as well...  */
> -	      new_macro.argv = (const char * const *) argv;
> -	    }
> -	  argv[new_macro.argc] = extract_identifier (&exp, 1).release ();
> -	  if (! argv[new_macro.argc])
> +	  auto arg = extract_identifier (&exp, 1);
> +	  if (! arg)

arg == nullptr

>  	    error (_("Macro is missing an argument."));
> -	  ++new_macro.argc;
>  
> -	  for (i = new_macro.argc - 2; i >= 0; --i)
> +	  for (const char *existing : new_macro.argv)
>  	    {
> -	      if (! strcmp (argv[i], argv[new_macro.argc - 1]))
> +	      if (! strcmp (existing, arg.get ()))
>  		error (_("Two macro arguments with identical names."));
>  	    }

You could remove the braces while at it.

Simon