From: Andrew Burgess <aburgess@redhat.com>
To: Tom Tromey via Gdb-patches <gdb-patches@sourceware.org>,
gdb-patches@sourceware.org
Cc: Tom Tromey <tromey@adacore.com>
Subject: Re: [PATCH 3/3] Use std::string for interpreter_p
Date: Tue, 21 Jun 2022 13:11:45 +0100 [thread overview]
Message-ID: <87iloucj26.fsf@redhat.com> (raw)
In-Reply-To: <20220620134650.2664575-4-tromey@adacore.com>
Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> The global interpreter_p is a manually-managed 'char *'. This patch
> changes it to be a std::string instead, and removes some erroneous
> comments.
I took a look through this series, and it all looks good to me.
I don't like the name interpreter_p - the _p says "predicate" to me, but
I that's not your fault, so can be fixed some other day.
Thanks,
Andrew
> ---
> gdb/interps.c | 11 ++---------
> gdb/main.c | 24 +++++++++---------------
> gdb/main.h | 2 +-
> gdb/tui/tui-interp.c | 9 +++------
> 4 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 0c440e78685..3a9c590b8c8 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -168,15 +168,8 @@ interp_set (struct interp *interp, bool top_level)
> if (top_level)
> ui_interp->top_level_interpreter = interp;
>
> - /* We use interpreter_p for the "set interpreter" variable, so we need
> - to make sure we have a malloc'ed copy for the set command to free. */
> - if (interpreter_p != NULL
> - && strcmp (interp->name (), interpreter_p) != 0)
> - {
> - xfree (interpreter_p);
> -
> - interpreter_p = xstrdup (interp->name ());
> - }
> + if (interpreter_p != interp->name ())
> + interpreter_p = interp->name ();
>
> /* Run the init proc. */
> if (!interp->inited)
> diff --git a/gdb/main.c b/gdb/main.c
> index ec2b7b01752..8c97987956b 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -56,10 +56,8 @@
> #include "observable.h"
> #include "serial.h"
>
> -/* The selected interpreter. This will be used as a set command
> - variable, so it should always be malloc'ed - since
> - do_setshow_command will free it. */
> -char *interpreter_p;
> +/* The selected interpreter. */
> +std::string interpreter_p;
>
> /* System root path, used to find libraries etc. */
> std::string gdb_sysroot;
> @@ -729,7 +727,7 @@ captured_main_1 (struct captured_main_args *context)
> this captured main, or one specified by the user at start up, or
> the console. Initialize the interpreter to the one requested by
> the application. */
> - interpreter_p = xstrdup (context->interpreter_p);
> + interpreter_p = context->interpreter_p;
>
> /* Parse arguments and options. */
> {
> @@ -866,8 +864,7 @@ captured_main_1 (struct captured_main_args *context)
> case OPT_TUI:
> /* --tui is equivalent to -i=tui. */
> #ifdef TUI
> - xfree (interpreter_p);
> - interpreter_p = xstrdup (INTERP_TUI);
> + interpreter_p = INTERP_TUI;
> #else
> error (_("%s: TUI mode is not supported"), gdb_program_name);
> #endif
> @@ -877,14 +874,12 @@ captured_main_1 (struct captured_main_args *context)
> actually useful, and if it is, what it should do. */
> #ifdef GDBTK
> /* --windows is equivalent to -i=insight. */
> - xfree (interpreter_p);
> - interpreter_p = xstrdup (INTERP_INSIGHT);
> + interpreter_p = INTERP_INSIGHT;
> #endif
> break;
> case OPT_NOWINDOWS:
> /* -nw is equivalent to -i=console. */
> - xfree (interpreter_p);
> - interpreter_p = xstrdup (INTERP_CONSOLE);
> + interpreter_p = INTERP_CONSOLE;
> break;
> case 'f':
> annotation_level = 1;
> @@ -950,8 +945,7 @@ captured_main_1 (struct captured_main_args *context)
> }
> #endif /* GDBTK */
> case 'i':
> - xfree (interpreter_p);
> - interpreter_p = xstrdup (optarg);
> + interpreter_p = optarg;
> break;
> case 'd':
> dirarg.push_back (optarg);
> @@ -1133,7 +1127,7 @@ captured_main_1 (struct captured_main_args *context)
> GDB retain the old MI1 interpreter startup behavior. Output the
> copyright message before the interpreter is installed. That way
> it isn't encapsulated in MI output. */
> - if (!quiet && strcmp (interpreter_p, INTERP_MI1) == 0)
> + if (!quiet && interpreter_p == INTERP_MI1)
> {
> /* Print all the junk at the top, with trailing "..." if we are
> about to read a symbol file (possibly slowly). */
> @@ -1147,7 +1141,7 @@ captured_main_1 (struct captured_main_args *context)
>
> /* Install the default UI. All the interpreters should have had a
> look at things by now. Initialize the default interpreter. */
> - set_top_level_interpreter (interpreter_p);
> + set_top_level_interpreter (interpreter_p.c_str ());
>
> /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets
> GDB retain the old MI1 interpreter startup behavior. Output the
> diff --git a/gdb/main.h b/gdb/main.h
> index 60b3569438e..7cdd93f3420 100644
> --- a/gdb/main.h
> +++ b/gdb/main.h
> @@ -36,7 +36,7 @@ extern int batch_silent;
> extern int batch_flag;
>
> /* * The name of the interpreter if specified on the command line. */
> -extern char *interpreter_p;
> +extern std::string interpreter_p;
>
> /* From mingw-hdep.c, used by main.c. */
>
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index e867441afb0..b4faede8ce6 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -343,14 +343,11 @@ _initialize_tui_interp ()
> {
> interp_factory_register (INTERP_TUI, tui_interp_factory);
>
> - if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0)
> + if (interpreter_p == INTERP_TUI)
> tui_start_enabled = true;
>
> - if (interpreter_p && strcmp (interpreter_p, INTERP_CONSOLE) == 0)
> - {
> - xfree (interpreter_p);
> - interpreter_p = xstrdup (INTERP_TUI);
> - }
> + if (interpreter_p == INTERP_CONSOLE)
> + interpreter_p = INTERP_TUI;
>
> /* If changing this, remember to update cli-interp.c as well. */
> gdb::observers::normal_stop.attach (tui_on_normal_stop, "tui-interp");
> --
> 2.34.1
next prev parent reply other threads:[~2022-06-21 12:11 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-20 13:46 [PATCH 0/3] Minor improvements to 'interp' Tom Tromey
2022-06-20 13:46 ` [PATCH 1/3] Use unique_xmalloc_ptr in interp Tom Tromey
2022-06-20 13:46 ` [PATCH 2/3] Move mi_interpreter to mi-interp.h Tom Tromey
2022-06-20 13:46 ` [PATCH 3/3] Use std::string for interpreter_p Tom Tromey
2022-06-21 12:11 ` Andrew Burgess [this message]
2022-06-22 19:50 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87iloucj26.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=tromey@adacore.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).