Re: [RFA 02/14] Introduce command_line_up

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-04-08 16:11, Tom Tromey wrote:
> This introduces command_line_up, a unique_ptr for command_line
> objects, and changes many places to use it.  This removes a number of
> cleanups.
>
> Command lines are funny in that sometimes they are reference counted.
> Once there is more C++-ification of some of the users, perhaps all of
> these can be changed to use shared_ptr instead.

Looks good to me in general, just a few comments.

> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 33b657d..ded2d2b 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -165,27 +165,22 @@ build_command_line (enum command_control_type
> type, const char *args)
>  /* Build and return a new command structure for the control commands
>     such as "if" and "while".  */
>
> -struct command_line *
> +command_line_up
>  get_command_line (enum command_control_type type, const char *arg)
>  {
> -  struct command_line *cmd;
> -  struct cleanup *old_chain = NULL;
> +  command_line_up cmd;
>
>    /* Allocate and build a new command line structure.  */
> -  cmd = build_command_line (type, arg);
> -
> -  old_chain = make_cleanup_free_command_lines (&cmd);
> +  cmd.reset (build_command_line (type, arg));

Can you do

  command_line_up cmd (build_command_line (type, arg));

?

> @@ -1256,17 +1243,17 @@ read_command_lines (char *prompt_arg, int
> from_tty, int parse_commands,
>  /* Act the same way as read_command_lines, except that each new line 
> is
>     obtained using READ_NEXT_LINE_FUNC.  */
>
> -struct command_line *
> +command_line_up
>  read_command_lines_1 (char * (*read_next_line_func) (void), int 
> parse_commands,
>  		      void (*validator)(char *, void *), void *closure)
>  {
> -  struct command_line *head, *tail, *next;
> -  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
> +  struct command_line *tail, *next;
> +  command_line_up head;
>    enum command_control_type ret;
>    enum misc_command_type val;
>
>    control_level = 0;
> -  head = tail = NULL;
> +  tail = NULL;
>
>    while (1)
>      {
> @@ -1307,18 +1294,17 @@ read_command_lines_1 (char *
> (*read_next_line_func) (void), int parse_commands,
>  	}
>        else
>  	{
> -	  head = next;
> -	  make_cleanup_free_command_lines (&head);
> +	  /* Make sure not to free HEAD.  */
> +	  head.release ();
> +	  head.reset (next);

I don't understand that bit, why the comment and the release?  From what 
I understand, this else clause is executed only for the first item, when 
head is still NULL.  The goal being to keep a reference to the head of 
the list.  All the other iterations will go in the true clause, which 
appends the new item at the end of the list.

>  	}
>        tail = next;
>      }
>
>    dont_repeat ();
>
> -  if (ret != invalid_control)
> -    discard_cleanups (old_chain);
> -  else
> -    do_cleanups (old_chain);
> +  if (ret == invalid_control)
> +    head.reset (NULL);

I wonder if a simple "return NULL" would do the trick here.  Either way 
is fine with me.

Thanks,

Simon