Re: [RFA] Use gdb::unique_xmalloc_ptr when calling tilde_expand

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

Hi Tom,

On 2017-08-03 23:43, Tom Tromey wrote:
> This patch changes most sites calling tilde_expand to use
> gdb::unique_xmalloc_ptr, rather than a cleanup.  It also changes
> scan_expression_with_cleanup to return a unique pointer, because the
> patch was already touching code in that area.

The patch looks good to me.  I noted two formatting nits (that were 
present before this patch), could you fix them before pushing?

> @@ -224,14 +210,12 @@ dump_memory_to_file (const char *cmd, const char
> *mode, const char *file_format)
>    /* Have everything.  Open/write the data.  */
>    if (file_format == NULL || strcmp (file_format, "binary") == 0)
>      {
> -      dump_binary_file (filename, mode, buf.data (), count);
> +      dump_binary_file (filename.get (), mode, buf.data (), count);
>      }
>    else
>      {
> -      dump_bfd_file (filename, mode, file_format, lo, buf.data (), 
> count);
> +      dump_bfd_file (filename.get (), mode, file_format, lo, buf.data
> (), count);
>      }

Remove these unnecessary curly braces.

> @@ -589,18 +568,18 @@ restore_command (char *args_in, int from_tty)
> 
>    if (info_verbose)
>      printf_filtered ("Restore file %s offset 0x%lx start 0x%lx end 
> 0x%lx\n",
> -		     filename, (unsigned long) data.load_offset,
> +		     filename.get (), (unsigned long) data.load_offset,
>  		     (unsigned long) data.load_start,
>  		     (unsigned long) data.load_end);
> 
>    if (binary_flag)
>      {
> -      restore_binary_file (filename, &data);
> +      restore_binary_file (filename.get (), &data);
>      }

Same here.


> diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
> index fb4283f..37bd96a 100644
> --- a/gdb/tracefile-tfile.c
> +++ b/gdb/tracefile-tfile.c
> @@ -423,7 +423,6 @@ static void
>  tfile_open (const char *arg, int from_tty)
>  {
>    char *temp;
> -  struct cleanup *old_chain;
>    int flags;
>    int scratch_chan;
>    char header[TRACE_HEADER_SIZE];
> @@ -433,34 +432,27 @@ tfile_open (const char *arg, int from_tty)
>    struct trace_status *ts;
>    struct uploaded_tp *uploaded_tps = NULL;
>    struct uploaded_tsv *uploaded_tsvs = NULL;
> -  char *filename;
> 
>    target_preopen (from_tty);
>    if (!arg)
>      error (_("No trace file specified."));
> 
> -  filename = tilde_expand (arg);
> -  if (!IS_ABSOLUTE_PATH(filename))
> -    {
> -      temp = concat (current_directory, "/", filename, (char *) NULL);
> -      xfree (filename);
> -      filename = temp;
> -    }
> -
> -  old_chain = make_cleanup (xfree, filename);
> +  gdb::unique_xmalloc_ptr<char> filename (tilde_expand (arg));
> +  if (!IS_ABSOLUTE_PATH (filename.get ()))
> +    filename.reset (concat (current_directory, "/", filename.get (),
> +			    (char *) NULL));
> 
>    flags = O_BINARY | O_LARGEFILE;
>    flags |= O_RDONLY;
> -  scratch_chan = gdb_open_cloexec (filename, flags, 0);
> +  scratch_chan = gdb_open_cloexec (filename.get (), flags, 0);
>    if (scratch_chan < 0)
> -    perror_with_name (filename);
> +    perror_with_name (filename.get ());
> 
>    /* Looks semi-reasonable.  Toss the old trace file and work on the 
> new.  */
> 
> -  discard_cleanups (old_chain);	/* Don't free filename any more.  */
>    unpush_target (&tfile_ops);
> 
> -  trace_filename = xstrdup (filename);
> +  trace_filename = filename.release ();

Hmm, the old code discarded the cleanup and did an xstrdup, that seems 
like a leak that you fixed :)

Thanks,

Simon