Re: [PATCH 3/3] Make relocate_{path,gdb_directory} return std::string

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Tue, Sep 10, 2019 at 10:27 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> This simplifies memory management. I've also changed some global variables
> Christian> to std::string accordingly (which store the result of these functions),
> Christian> but not all because some are used with add_setshow_optional_filename_cmd
> Christian> which requires a char*.
>
> Thank you.

Thanks for the review!

> Christian> -  xfree (gdb_datadir);
> Christian> -  gdb_datadir = gdb_realpath (new_datadir).release ();
> Christian> +  gdb_datadir = gdb_realpath (new_datadir).get ();
>
> I wonder if using a unique_xmalloc_ptr would be better.
> I suppose it doesn't matter hugely, but it would eliminate some of these
> double allocations.

Do you mean for the return type of relocate_path and
relocate_gdb_directory? Hm.. I can change it if you want, but I'd
rather not -- not all codepaths go through an xmalloc (when
relocatable=false, or relocate_path returns null in
relocate_gdb_directory). So that would add some xstrdup calls that
would not otherwise be necessary.

> Christian> +      gdb::unique_xmalloc_ptr<char> abs_datadir =
> Christian> +    gdb_abspath (gdb_datadir.c_str ());
>
> "=" on continuation line.

Done.

> Christian> -static char *
> Christian> +static std::string
> Christian>  relocate_path (const char *progname, const char *initial, bool relocatable)
> Christian>  {
> Christian>    if (relocatable)
> Christian> -    return make_relative_prefix (progname, BINDIR, initial);
> Christian> -  return xstrdup (initial);
> Christian> +    {
> Christian> +      char *str = make_relative_prefix (progname, BINDIR, initial);
> Christian> +      if (str != nullptr)
> Christian> +        return str;
>
> This seems to leak "str".

Oh, thanks! Fixed using a unique_xmalloc_ptr.

> Christian> +      char *canon_sysroot = lrealpath (dir.c_str ());
>
> Changing this to a unique_xmalloc_ptr would mean one less call to
> xfree...

Thanks, done.

> Christian> +  debug_file_directory =
>
> "=" on the next line, there is one more of these in this spot.

Done.

I'll send a new version with those comments fixed in a moment.

Christian