Re: [RFA 3/3] Make extract_arg return a std::string

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

On 2017-09-11 02:33, Tom Tromey wrote:
> Change extract_arg to return a std::string and fix up all the users.
> I think string is mildly better than unique_xmalloc_ptr<char>, when
> possible, because it provides a more robust API.

I agree, for strings we allocate dynamically ourselves, it makes sense 
to use std::string.  For dynamically allocated strings coming from 
external APIs we use, then unique_xmalloc_ptr<char> make sense, since it 
doesn't require doing a copy.  For things that are always 
constant/literal strings, the const char *.

> I changed the error messages emitted from find_location_by_number to
> avoid either writing to a string or an extra allocation; this can be
> changed but I thought that the new message was not any less clear.
> You can see an example in the testsuite patch.

I made almost the same patch (although unpolished):

https://github.com/simark/binutils-gdb/commit/afac80f165a016ca2089169efe7dd266aa8a0ddc

and made pretty much the same choices, so that's fine with me.  I just 
have some small comments, the patch is ok to me with those fixed.

> diff --git a/gdb/break-catch-sig.c b/gdb/break-catch-sig.c
> index 805084f..237dbaa 100644
> --- a/gdb/break-catch-sig.c
> +++ b/gdb/break-catch-sig.c
> @@ -343,12 +343,12 @@ catch_signal_split_args (char *arg, bool 
> *catch_all)
>        gdb_signal signal_number;
>        char *endptr;
>
> -      gdb::unique_xmalloc_ptr<char> one_arg (extract_arg (&arg));
> -      if (one_arg == NULL)
> +      std::string one_arg = extract_arg (&arg);
> +      if (one_arg.empty ())
>  	break;
>
>        /* Check for the special flag "all".  */
> -      if (strcmp (one_arg.get (), "all") == 0)
> +      if (strcmp (one_arg.c_str (), "all") == 0)

one_arg == "all"

> diff --git a/gdb/cli/cli-utils.h b/gdb/cli/cli-utils.h
> index 038ddad..c17b4dd 100644
> --- a/gdb/cli/cli-utils.h
> +++ b/gdb/cli/cli-utils.h
> @@ -149,17 +149,14 @@ remove_trailing_whitespace (const char *start, 
> char *s)
>  }
>
>  /* A helper function to extract an argument from *ARG.  An argument is
> -   delimited by whitespace.  The return value is either NULL if no
> -   argument was found, or an xmalloc'd string.  */
> +   delimited by whitespace.  The return value is either empty if no

"either" does not make sense in the new sentence.

> +   argument was found.  */
>
> -extern char *extract_arg (char **arg);
> +extern std::string extract_arg (char **arg);
>
> -/* A const-correct version of "extract_arg".
> +/* A const-correct version of "extract_arg".  */

It's a bit funny to refer to extract_arg when this function is also 
named extract_arg.  What about: "A const-correct version of the above." 
?

> @@ -553,16 +556,14 @@ exists_probe_with_pops (VEC (bound_probe_s) 
> *probes,
>     [PROBE [OBJNAME]]] from the provided string STR.  */
>
>  static void
> -parse_probe_linespec (const char *str, char **provider,
> -		      char **probe_name, char **objname)
> +parse_probe_linespec (const char *str, std::string *provider,
> +		      std::string *probe_name, std::string *objname)
>  {
> -  *probe_name = *objname = NULL;

Here, I would keep the clearing of probe_name and objname (calling 
.clear ()), just in case a user of parse_probe_linespec decides to 
re-use string objects.  It then couldn't know if parse_probe_linespec 
set the value, or if it's the old value.

Simon