* [PATCH 0/3] Some minor interps tweaks
@ 2023-03-02 20:32 Simon Marchi
2023-03-02 20:32 ` [PATCH 1/3] gdb: make get_interp_info return a reference Simon Marchi
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Simon Marchi @ 2023-03-02 20:32 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
As I'm looking through interps.c doing a larger change, I came up with
these small cleanups.
Simon Marchi (3):
gdb: make get_interp_info return a reference
gdb: make interp::m_name an `const char *`
gdb: initialize interp::next
gdb/interps.c | 79 +++++++++++++++++++++++++--------------------------
gdb/interps.h | 14 ++++-----
2 files changed, 45 insertions(+), 48 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-02 20:32 [PATCH 0/3] Some minor interps tweaks Simon Marchi @ 2023-03-02 20:32 ` Simon Marchi 2023-03-07 19:27 ` Tom Tromey 2023-03-02 20:32 ` [PATCH 2/3] gdb: make interp::m_name an `const char *` Simon Marchi 2023-03-02 20:32 ` [PATCH 3/3] gdb: initialize interp::next Simon Marchi 2 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2023-03-02 20:32 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi get_interp_info and get_current_interp_info always return non-nullptr, so they can return a reference instead of a pointer. Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0 --- gdb/interps.c | 71 +++++++++++++++++++++++++-------------------------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 9c7908bde1ca..9517e57540d3 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -55,20 +55,19 @@ struct ui_interp_info struct interp *command_interpreter; }; -/* Get UI's ui_interp_info object. Never returns NULL. */ +/* Get UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_interp_info (struct ui *ui) { if (ui->interp_info == NULL) ui->interp_info = XCNEW (struct ui_interp_info); - return ui->interp_info; + return *ui->interp_info; } -/* Get the current UI's ui_interp_info object. Never returns - NULL. */ +/* Get the current UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_current_interp_info (void) { return get_interp_info (current_ui); @@ -128,12 +127,12 @@ interp_factory_register (const char *name, interp_factory_func func) static void interp_add (struct ui *ui, struct interp *interp) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL); - interp->next = ui_interp->interp_list; - ui_interp->interp_list = interp; + interp->next = ui_interp.interp_list; + ui_interp.interp_list = interp; } /* This sets the current interpreter to be INTERP. If INTERP has not @@ -150,13 +149,13 @@ interp_add (struct ui *ui, struct interp *interp) static void interp_set (struct interp *interp, bool top_level) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *old_interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *old_interp = ui_interp.current_interpreter; /* If we already have an interpreter, then trying to set top level interpreter is kinda pointless. */ - gdb_assert (!top_level || !ui_interp->current_interpreter); - gdb_assert (!top_level || !ui_interp->top_level_interpreter); + gdb_assert (!top_level || !ui_interp.current_interpreter); + gdb_assert (!top_level || !ui_interp.top_level_interpreter); if (old_interp != NULL) { @@ -164,9 +163,9 @@ interp_set (struct interp *interp, bool top_level) old_interp->suspend (); } - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; if (top_level) - ui_interp->top_level_interpreter = interp; + ui_interp.top_level_interpreter = interp; if (interpreter_p != interp->name ()) interpreter_p = interp->name (); @@ -203,10 +202,10 @@ interp_set (struct interp *interp, bool top_level) static struct interp * interp_lookup_existing (struct ui *ui, const char *name) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); struct interp *interp; - for (interp = ui_interp->interp_list; + for (interp = ui_interp.interp_list; interp != NULL; interp = interp->next) { @@ -259,8 +258,8 @@ void current_interp_set_logging (ui_file_up logfile, bool logging_redirect, bool debug_redirect) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; interp->set_logging (std::move (logfile), logging_redirect, debug_redirect); } @@ -269,12 +268,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect, struct interp * scoped_restore_interp::set_interp (const char *name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *interp = interp_lookup (current_ui, name); - struct interp *old_interp = ui_interp->current_interpreter; + struct interp *old_interp = ui_interp.current_interpreter; if (interp) - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; return old_interp; } @@ -282,8 +281,8 @@ scoped_restore_interp::set_interp (const char *name) int current_interp_named_p (const char *interp_name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; if (interp != NULL) return (strcmp (interp->name (), interp_name) == 0); @@ -304,12 +303,12 @@ current_interp_named_p (const char *interp_name) struct interp * command_interp (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - if (ui_interp->command_interpreter != NULL) - return ui_interp->command_interpreter; + if (ui_interp.command_interpreter != NULL) + return ui_interp.command_interpreter; else - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* See interps.h. */ @@ -336,11 +335,11 @@ interp_supports_command_editing (struct interp *interp) void interp_exec (struct interp *interp, const char *command_str) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); /* See `command_interp' for why we do this. */ scoped_restore save_command_interp - = make_scoped_restore (&ui_interp->command_interpreter, interp); + = make_scoped_restore (&ui_interp.command_interpreter, interp); interp->exec (command_str); } @@ -365,7 +364,7 @@ clear_interpreter_hooks (void) static void interpreter_exec_cmd (const char *args, int from_tty) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *old_interp, *interp_to_use; unsigned int nrules; unsigned int i; @@ -387,7 +386,7 @@ interpreter_exec_cmd (const char *args, int from_tty) if (nrules < 2) error (_("Usage: interpreter-exec INTERPRETER COMMAND...")); - old_interp = ui_interp->current_interpreter; + old_interp = ui_interp.current_interpreter; interp_to_use = interp_lookup (current_ui, prules[0]); if (interp_to_use == NULL) @@ -425,9 +424,9 @@ interpreter_completer (struct cmd_list_element *ignore, struct interp * top_level_interpreter (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - return ui_interp->top_level_interpreter; + return ui_interp.top_level_interpreter; } /* See interps.h. */ @@ -435,9 +434,9 @@ top_level_interpreter (void) struct interp * current_interpreter (void) { - struct ui_interp_info *ui_interp = get_interp_info (current_ui); + ui_interp_info &ui_interp = get_interp_info (current_ui); - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* This just adds the "interpreter-exec" command. */ -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-02 20:32 ` [PATCH 1/3] gdb: make get_interp_info return a reference Simon Marchi @ 2023-03-07 19:27 ` Tom Tromey 2023-03-07 20:34 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Tom Tromey @ 2023-03-07 19:27 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> get_interp_info and get_current_interp_info always return non-nullptr, Simon> so they can return a reference instead of a pointer. Maybe it would be better here if ui_interp_info also disabled copying. Otherwise it seems easy to accidentally drop a "&" and wind up modifying a new copy. Well, that's my first reaction anyway. I'm not sure how much we should really be worried about it. Tom ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-07 19:27 ` Tom Tromey @ 2023-03-07 20:34 ` Simon Marchi 2023-03-07 20:45 ` Tom Tromey 0 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2023-03-07 20:34 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 3/7/23 14:27, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> get_interp_info and get_current_interp_info always return non-nullptr, > Simon> so they can return a reference instead of a pointer. > > Maybe it would be better here if ui_interp_info also disabled copying. > Otherwise it seems easy to accidentally drop a "&" and wind up modifying > a new copy. > > Well, that's my first reaction anyway. I'm not sure how much we should > really be worried about it. Well, it's the language we have to work with, it does what we ask it to do. Here, we don't need for ui_interp_info to be copyable, to we can do it, but if we needed it to be copyable, it would be down to being careful. Here's what the patch looks like now: From e446fa8ad63f2a33f6f7ffb2427d00998da3c781 Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@efficios.com> Date: Thu, 2 Mar 2023 15:32:22 -0500 Subject: [PATCH] gdb: make get_interp_info return a reference get_interp_info and get_current_interp_info always return non-nullptr, so they can return a reference instead of a pointer. Since we don't need to copy it, make ui_interp_info non-copyiable, to avoid a copying it in a local variable, instead of getting a reference. Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0 --- gdb/interps.c | 76 ++++++++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 9c7908bde1ca..9768f3cf8804 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -45,6 +45,9 @@ struct ui_interp_info { + ui_interp_info () = default; + DISABLE_COPY_AND_ASSIGN(ui_interp_info); + /* Each top level has its own independent set of interpreters. */ struct interp *interp_list; struct interp *current_interpreter; @@ -55,20 +58,19 @@ struct ui_interp_info struct interp *command_interpreter; }; -/* Get UI's ui_interp_info object. Never returns NULL. */ +/* Get UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_interp_info (struct ui *ui) { if (ui->interp_info == NULL) - ui->interp_info = XCNEW (struct ui_interp_info); - return ui->interp_info; + ui->interp_info = new ui_interp_info; + return *ui->interp_info; } -/* Get the current UI's ui_interp_info object. Never returns - NULL. */ +/* Get the current UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_current_interp_info (void) { return get_interp_info (current_ui); @@ -128,12 +130,12 @@ interp_factory_register (const char *name, interp_factory_func func) static void interp_add (struct ui *ui, struct interp *interp) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL); - interp->next = ui_interp->interp_list; - ui_interp->interp_list = interp; + interp->next = ui_interp.interp_list; + ui_interp.interp_list = interp; } /* This sets the current interpreter to be INTERP. If INTERP has not @@ -150,13 +152,13 @@ interp_add (struct ui *ui, struct interp *interp) static void interp_set (struct interp *interp, bool top_level) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *old_interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *old_interp = ui_interp.current_interpreter; /* If we already have an interpreter, then trying to set top level interpreter is kinda pointless. */ - gdb_assert (!top_level || !ui_interp->current_interpreter); - gdb_assert (!top_level || !ui_interp->top_level_interpreter); + gdb_assert (!top_level || !ui_interp.current_interpreter); + gdb_assert (!top_level || !ui_interp.top_level_interpreter); if (old_interp != NULL) { @@ -164,9 +166,9 @@ interp_set (struct interp *interp, bool top_level) old_interp->suspend (); } - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; if (top_level) - ui_interp->top_level_interpreter = interp; + ui_interp.top_level_interpreter = interp; if (interpreter_p != interp->name ()) interpreter_p = interp->name (); @@ -203,10 +205,10 @@ interp_set (struct interp *interp, bool top_level) static struct interp * interp_lookup_existing (struct ui *ui, const char *name) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); struct interp *interp; - for (interp = ui_interp->interp_list; + for (interp = ui_interp.interp_list; interp != NULL; interp = interp->next) { @@ -259,8 +261,8 @@ void current_interp_set_logging (ui_file_up logfile, bool logging_redirect, bool debug_redirect) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; interp->set_logging (std::move (logfile), logging_redirect, debug_redirect); } @@ -269,12 +271,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect, struct interp * scoped_restore_interp::set_interp (const char *name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *interp = interp_lookup (current_ui, name); - struct interp *old_interp = ui_interp->current_interpreter; + struct interp *old_interp = ui_interp.current_interpreter; if (interp) - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; return old_interp; } @@ -282,8 +284,8 @@ scoped_restore_interp::set_interp (const char *name) int current_interp_named_p (const char *interp_name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; if (interp != NULL) return (strcmp (interp->name (), interp_name) == 0); @@ -304,12 +306,12 @@ current_interp_named_p (const char *interp_name) struct interp * command_interp (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - if (ui_interp->command_interpreter != NULL) - return ui_interp->command_interpreter; + if (ui_interp.command_interpreter != NULL) + return ui_interp.command_interpreter; else - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* See interps.h. */ @@ -336,11 +338,11 @@ interp_supports_command_editing (struct interp *interp) void interp_exec (struct interp *interp, const char *command_str) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); /* See `command_interp' for why we do this. */ scoped_restore save_command_interp - = make_scoped_restore (&ui_interp->command_interpreter, interp); + = make_scoped_restore (&ui_interp.command_interpreter, interp); interp->exec (command_str); } @@ -365,7 +367,7 @@ clear_interpreter_hooks (void) static void interpreter_exec_cmd (const char *args, int from_tty) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *old_interp, *interp_to_use; unsigned int nrules; unsigned int i; @@ -387,7 +389,7 @@ interpreter_exec_cmd (const char *args, int from_tty) if (nrules < 2) error (_("Usage: interpreter-exec INTERPRETER COMMAND...")); - old_interp = ui_interp->current_interpreter; + old_interp = ui_interp.current_interpreter; interp_to_use = interp_lookup (current_ui, prules[0]); if (interp_to_use == NULL) @@ -425,9 +427,9 @@ interpreter_completer (struct cmd_list_element *ignore, struct interp * top_level_interpreter (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - return ui_interp->top_level_interpreter; + return ui_interp.top_level_interpreter; } /* See interps.h. */ @@ -435,9 +437,9 @@ top_level_interpreter (void) struct interp * current_interpreter (void) { - struct ui_interp_info *ui_interp = get_interp_info (current_ui); + ui_interp_info &ui_interp = get_interp_info (current_ui); - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* This just adds the "interpreter-exec" command. */ base-commit: 4779ed9757fa71e6743fb7fc1f9eeae8267ae36c -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-07 20:34 ` Simon Marchi @ 2023-03-07 20:45 ` Tom Tromey 2023-03-07 20:52 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Tom Tromey @ 2023-03-07 20:45 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi, Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> Here's what the patch looks like now: Simon> if (ui->interp_info == NULL) Simon> - ui->interp_info = XCNEW (struct ui_interp_info); Simon> - return ui->interp_info; Simon> + ui->interp_info = new ui_interp_info; Unfortunately XCNEW -> new also means adding member initializers to ensure they are 0. Tom ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-07 20:45 ` Tom Tromey @ 2023-03-07 20:52 ` Simon Marchi 2023-03-07 21:29 ` Tom Tromey 0 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2023-03-07 20:52 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 3/7/23 15:45, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> Here's what the patch looks like now: > > Simon> if (ui->interp_info == NULL) > Simon> - ui->interp_info = XCNEW (struct ui_interp_info); > Simon> - return ui->interp_info; > Simon> + ui->interp_info = new ui_interp_info; > > Unfortunately XCNEW -> new also means adding member initializers to > ensure they are 0. Didn't I? Oh, I did but it's uncommitted in my git tree. I swear! Here's the updated patch. From 67119cc8ca5e2d4729f39455829fdc5fa095932a Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@efficios.com> Date: Thu, 2 Mar 2023 15:32:22 -0500 Subject: [PATCH] gdb: make get_interp_info return a reference get_interp_info and get_current_interp_info always return non-nullptr, so they can return a reference instead of a pointer. Since we don't need to copy it, make ui_interp_info non-copyiable, to avoid a copying it in a local variable, instead of getting a reference. Change-Id: I6d8dea92dc26a58ea340d04862db6b8d9cf906a0 --- gdb/interps.c | 84 ++++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 9c7908bde1ca..12a0048d1020 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -45,30 +45,32 @@ struct ui_interp_info { + ui_interp_info () = default; + DISABLE_COPY_AND_ASSIGN (ui_interp_info); + /* Each top level has its own independent set of interpreters. */ - struct interp *interp_list; - struct interp *current_interpreter; - struct interp *top_level_interpreter; + interp *interp_list = nullptr; + interp *current_interpreter = nullptr; + interp *top_level_interpreter = nullptr; /* The interpreter that is active while `interp_exec' is active, NULL at all other times. */ - struct interp *command_interpreter; + interp *command_interpreter = nullptr; }; -/* Get UI's ui_interp_info object. Never returns NULL. */ +/* Get UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_interp_info (struct ui *ui) { if (ui->interp_info == NULL) - ui->interp_info = XCNEW (struct ui_interp_info); - return ui->interp_info; + ui->interp_info = new ui_interp_info; + return *ui->interp_info; } -/* Get the current UI's ui_interp_info object. Never returns - NULL. */ +/* Get the current UI's ui_interp_info object. */ -static struct ui_interp_info * +static ui_interp_info & get_current_interp_info (void) { return get_interp_info (current_ui); @@ -128,12 +130,12 @@ interp_factory_register (const char *name, interp_factory_func func) static void interp_add (struct ui *ui, struct interp *interp) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); gdb_assert (interp_lookup_existing (ui, interp->name ()) == NULL); - interp->next = ui_interp->interp_list; - ui_interp->interp_list = interp; + interp->next = ui_interp.interp_list; + ui_interp.interp_list = interp; } /* This sets the current interpreter to be INTERP. If INTERP has not @@ -150,13 +152,13 @@ interp_add (struct ui *ui, struct interp *interp) static void interp_set (struct interp *interp, bool top_level) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *old_interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *old_interp = ui_interp.current_interpreter; /* If we already have an interpreter, then trying to set top level interpreter is kinda pointless. */ - gdb_assert (!top_level || !ui_interp->current_interpreter); - gdb_assert (!top_level || !ui_interp->top_level_interpreter); + gdb_assert (!top_level || !ui_interp.current_interpreter); + gdb_assert (!top_level || !ui_interp.top_level_interpreter); if (old_interp != NULL) { @@ -164,9 +166,9 @@ interp_set (struct interp *interp, bool top_level) old_interp->suspend (); } - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; if (top_level) - ui_interp->top_level_interpreter = interp; + ui_interp.top_level_interpreter = interp; if (interpreter_p != interp->name ()) interpreter_p = interp->name (); @@ -203,10 +205,10 @@ interp_set (struct interp *interp, bool top_level) static struct interp * interp_lookup_existing (struct ui *ui, const char *name) { - struct ui_interp_info *ui_interp = get_interp_info (ui); + ui_interp_info &ui_interp = get_interp_info (ui); struct interp *interp; - for (interp = ui_interp->interp_list; + for (interp = ui_interp.interp_list; interp != NULL; interp = interp->next) { @@ -259,8 +261,8 @@ void current_interp_set_logging (ui_file_up logfile, bool logging_redirect, bool debug_redirect) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; interp->set_logging (std::move (logfile), logging_redirect, debug_redirect); } @@ -269,12 +271,12 @@ current_interp_set_logging (ui_file_up logfile, bool logging_redirect, struct interp * scoped_restore_interp::set_interp (const char *name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *interp = interp_lookup (current_ui, name); - struct interp *old_interp = ui_interp->current_interpreter; + struct interp *old_interp = ui_interp.current_interpreter; if (interp) - ui_interp->current_interpreter = interp; + ui_interp.current_interpreter = interp; return old_interp; } @@ -282,8 +284,8 @@ scoped_restore_interp::set_interp (const char *name) int current_interp_named_p (const char *interp_name) { - struct ui_interp_info *ui_interp = get_current_interp_info (); - struct interp *interp = ui_interp->current_interpreter; + ui_interp_info &ui_interp = get_current_interp_info (); + struct interp *interp = ui_interp.current_interpreter; if (interp != NULL) return (strcmp (interp->name (), interp_name) == 0); @@ -304,12 +306,12 @@ current_interp_named_p (const char *interp_name) struct interp * command_interp (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - if (ui_interp->command_interpreter != NULL) - return ui_interp->command_interpreter; + if (ui_interp.command_interpreter != NULL) + return ui_interp.command_interpreter; else - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* See interps.h. */ @@ -336,11 +338,11 @@ interp_supports_command_editing (struct interp *interp) void interp_exec (struct interp *interp, const char *command_str) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); /* See `command_interp' for why we do this. */ scoped_restore save_command_interp - = make_scoped_restore (&ui_interp->command_interpreter, interp); + = make_scoped_restore (&ui_interp.command_interpreter, interp); interp->exec (command_str); } @@ -365,7 +367,7 @@ clear_interpreter_hooks (void) static void interpreter_exec_cmd (const char *args, int from_tty) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); struct interp *old_interp, *interp_to_use; unsigned int nrules; unsigned int i; @@ -387,7 +389,7 @@ interpreter_exec_cmd (const char *args, int from_tty) if (nrules < 2) error (_("Usage: interpreter-exec INTERPRETER COMMAND...")); - old_interp = ui_interp->current_interpreter; + old_interp = ui_interp.current_interpreter; interp_to_use = interp_lookup (current_ui, prules[0]); if (interp_to_use == NULL) @@ -425,9 +427,9 @@ interpreter_completer (struct cmd_list_element *ignore, struct interp * top_level_interpreter (void) { - struct ui_interp_info *ui_interp = get_current_interp_info (); + ui_interp_info &ui_interp = get_current_interp_info (); - return ui_interp->top_level_interpreter; + return ui_interp.top_level_interpreter; } /* See interps.h. */ @@ -435,9 +437,9 @@ top_level_interpreter (void) struct interp * current_interpreter (void) { - struct ui_interp_info *ui_interp = get_interp_info (current_ui); + ui_interp_info &ui_interp = get_interp_info (current_ui); - return ui_interp->current_interpreter; + return ui_interp.current_interpreter; } /* This just adds the "interpreter-exec" command. */ base-commit: 4779ed9757fa71e6743fb7fc1f9eeae8267ae36c -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-07 20:52 ` Simon Marchi @ 2023-03-07 21:29 ` Tom Tromey 2023-03-07 21:31 ` Simon Marchi 0 siblings, 1 reply; 12+ messages in thread From: Tom Tromey @ 2023-03-07 21:29 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Tom Tromey, Simon Marchi, Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> Didn't I? Oh, I did but it's uncommitted in my git tree. I swear! Been there myself. Simon> Here's the updated patch. Thanks. Reviewed-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] gdb: make get_interp_info return a reference 2023-03-07 21:29 ` Tom Tromey @ 2023-03-07 21:31 ` Simon Marchi 0 siblings, 0 replies; 12+ messages in thread From: Simon Marchi @ 2023-03-07 21:31 UTC (permalink / raw) To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Simon Marchi On 3/7/23 16:29, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > > Simon> Didn't I? Oh, I did but it's uncommitted in my git tree. I swear! > > Been there myself. > > Simon> Here's the updated patch. > > Thanks. > Reviewed-By: Tom Tromey <tom@tromey.com> > > Tom Thanks, pushed the whole series. Simon ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] gdb: make interp::m_name an `const char *` 2023-03-02 20:32 [PATCH 0/3] Some minor interps tweaks Simon Marchi 2023-03-02 20:32 ` [PATCH 1/3] gdb: make get_interp_info return a reference Simon Marchi @ 2023-03-02 20:32 ` Simon Marchi 2023-03-07 19:29 ` Tom Tromey 2023-03-02 20:32 ` [PATCH 3/3] gdb: initialize interp::next Simon Marchi 2 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2023-03-02 20:32 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi From: Simon Marchi <simon.marchi@polymtl.ca> I realized that the memory for interp names does not need to be allocated. The name used to register interp factory functions is always a literal string, so has static storage duration. If we change interp_lookup to pass that name instead of the string that it receives as a parameter (which does not always have static storage duration), then interps can simply store pointers to the name. So, change interp_lookup to pass `factory.name` rather than `name`. Change interp::m_name to be a `const char *` rather than an std::string. Change-Id: I0474d1f7b3512e7d172ccd73018aea927def3188 --- gdb/interps.c | 8 +++----- gdb/interps.h | 12 ++++++------ 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/gdb/interps.c b/gdb/interps.c index 9517e57540d3..5b964aebbbd6 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -79,13 +79,11 @@ static struct interp *interp_lookup_existing (struct ui *ui, const char *name); interp::interp (const char *name) - : m_name (make_unique_xstrdup (name)) + : m_name (name) { } -interp::~interp () -{ -} +interp::~interp () = default; /* An interpreter factory. Maps an interpreter name to the factory function that instantiates an interpreter by that name. */ @@ -232,7 +230,7 @@ interp_lookup (struct ui *ui, const char *name) for (const interp_factory &factory : interpreter_factories) if (strcmp (factory.name, name) == 0) { - interp = factory.func (name); + interp = factory.func (factory.name); interp_add (ui, interp); return interp; } diff --git a/gdb/interps.h b/gdb/interps.h index 01bec47550dd..62f37951ddea 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -32,7 +32,9 @@ typedef struct interp *(*interp_factory_func) (const char *name); /* Each interpreter kind (CLI, MI, etc.) registers itself with a call to this function, passing along its name, and a pointer to a function that creates a new instance of an interpreter with that - name. */ + name. + + The memory for NAME must have static storage duration. */ extern void interp_factory_register (const char *name, interp_factory_func func); @@ -76,13 +78,11 @@ class interp { return false; } const char *name () const - { - return m_name.get (); - } + { return m_name; } private: - /* This is the name in "-i=" and "set interpreter". */ - gdb::unique_xmalloc_ptr<char> m_name; + /* The memory for this is static, it comes from literal strings (e.g. "cli"). */ + const char *m_name; public: /* Interpreters are stored in a linked list, this is the next -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] gdb: make interp::m_name an `const char *` 2023-03-02 20:32 ` [PATCH 2/3] gdb: make interp::m_name an `const char *` Simon Marchi @ 2023-03-07 19:29 ` Tom Tromey 0 siblings, 0 replies; 12+ messages in thread From: Tom Tromey @ 2023-03-07 19:29 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi, Simon Marchi >>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: Simon> I realized that the memory for interp names does not need to be Simon> allocated. The name used to register interp factory functions is always Simon> a literal string, so has static storage duration. If we change Simon> interp_lookup to pass that name instead of the string that it receives Simon> as a parameter (which does not always have static storage duration), Simon> then interps can simply store pointers to the name. I wish we had a way to detect/enforce this. That way lies rust I suppose. Simon> So, change interp_lookup to pass `factory.name` rather than `name`. Simon> Change interp::m_name to be a `const char *` rather than an std::string. Reviewed-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] gdb: initialize interp::next 2023-03-02 20:32 [PATCH 0/3] Some minor interps tweaks Simon Marchi 2023-03-02 20:32 ` [PATCH 1/3] gdb: make get_interp_info return a reference Simon Marchi 2023-03-02 20:32 ` [PATCH 2/3] gdb: make interp::m_name an `const char *` Simon Marchi @ 2023-03-02 20:32 ` Simon Marchi 2023-03-07 19:29 ` Tom Tromey 2 siblings, 1 reply; 12+ messages in thread From: Simon Marchi @ 2023-03-02 20:32 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi From: Simon Marchi <simon.marchi@polymtl.ca> This field is never initialized, it seems to me like it would be a good idea to initialize it to nullptr to avoid bad surprises. Change-Id: I8c04319d564f5d385d8bf0acee758f6ce28b4447 --- gdb/interps.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/interps.h b/gdb/interps.h index 62f37951ddea..29248093c671 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -87,7 +87,7 @@ class interp public: /* Interpreters are stored in a linked list, this is the next one... */ - struct interp *next; + interp *next = nullptr; /* Has the init method been run? */ bool inited = false; -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] gdb: initialize interp::next 2023-03-02 20:32 ` [PATCH 3/3] gdb: initialize interp::next Simon Marchi @ 2023-03-07 19:29 ` Tom Tromey 0 siblings, 0 replies; 12+ messages in thread From: Tom Tromey @ 2023-03-07 19:29 UTC (permalink / raw) To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi, Simon Marchi Simon> This field is never initialized, it seems to me like it would be a good Simon> idea to initialize it to nullptr to avoid bad surprises. Seems totally fine. Reviewed-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-07 21:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-02 20:32 [PATCH 0/3] Some minor interps tweaks Simon Marchi 2023-03-02 20:32 ` [PATCH 1/3] gdb: make get_interp_info return a reference Simon Marchi 2023-03-07 19:27 ` Tom Tromey 2023-03-07 20:34 ` Simon Marchi 2023-03-07 20:45 ` Tom Tromey 2023-03-07 20:52 ` Simon Marchi 2023-03-07 21:29 ` Tom Tromey 2023-03-07 21:31 ` Simon Marchi 2023-03-02 20:32 ` [PATCH 2/3] gdb: make interp::m_name an `const char *` Simon Marchi 2023-03-07 19:29 ` Tom Tromey 2023-03-02 20:32 ` [PATCH 3/3] gdb: initialize interp::next Simon Marchi 2023-03-07 19:29 ` Tom Tromey
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).