[RFA 3/3] Make extract_arg return a std::string
Simon Marchi
simon.marchi@polymtl.ca
Mon Sep 11 19:52:00 GMT 2017
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
More information about the Gdb-patches
mailing list