Re: [PATCH 2/6] gdb: make use of std::string in utils.c

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-08-19 14:41:41 -0400]:

> 
> 
> On 2021-08-19 5:49 a.m., Andrew Burgess wrote:
> > Replace use of manual string management (malloc/free) with std::string
> > when creating commands in utils.c.  Things are a little bit messy as
> > creating the prefix commands (using add_basic_prefix_cmd and
> > add_show_prefix_cmd), don't copy the doc string, while creating the
> > actual set/show commands does copy the doc string, this explains the
> > extra xstrdup when creating the prefix commands.
> 
> Indeed, all of this is really not consistent.  I guess we would need to
> create versions of the add_*_cmd that take a
> gdb::unique_xmalloc_pointer<char> and set
> cmd_list_element::doc_allocated.
> 
> > There should be no user visible changes after this commit.
> > ---
> >  gdb/utils.c | 58 ++++++++++++++++++++++++-----------------------------
> >  1 file changed, 26 insertions(+), 32 deletions(-)
> > 
> > diff --git a/gdb/utils.c b/gdb/utils.c
> > index 3916ae5a1c9..143c2eddd70 100644
> > --- a/gdb/utils.c
> > +++ b/gdb/utils.c
> > @@ -508,72 +508,66 @@ add_internal_problem_command (struct internal_problem *problem)
> >  {
> >    struct cmd_list_element **set_cmd_list;
> >    struct cmd_list_element **show_cmd_list;
> > -  char *set_doc;
> > -  char *show_doc;
> >  
> >    set_cmd_list = XNEW (struct cmd_list_element *);
> >    show_cmd_list = XNEW (struct cmd_list_element *);
> >    *set_cmd_list = NULL;
> >    *show_cmd_list = NULL;
> >  
> > -  set_doc = xstrprintf (_("Configure what GDB does when %s is detected."),
> > -			problem->name);
> > +  std::string set_doc
> > +    = string_printf (_("Configure what GDB does when %s is detected."),
> > +		       problem->name);
> >  
> > -  show_doc = xstrprintf (_("Show what GDB does when %s is detected."),
> > -			 problem->name);
> > +  std::string show_doc
> > +    = string_printf (_("Show what GDB does when %s is detected."),
> > +		       problem->name);
> >  
> > -  add_basic_prefix_cmd (problem->name, class_maintenance, set_doc,
> > -			set_cmd_list,
> > +  add_basic_prefix_cmd (problem->name, class_maintenance,
> > +			xstrdup (set_doc.c_str ()), set_cmd_list,
> >  			0/*allow-unknown*/, &maintenance_set_cmdlist);
> >  
> > -  add_show_prefix_cmd (problem->name, class_maintenance, show_doc,
> > -		       show_cmd_list,
> > +  add_show_prefix_cmd (problem->name, class_maintenance,
> > +		       xstrdup (show_doc.c_str ()), show_cmd_list,
> >  		       0/*allow-unknown*/, &maintenance_show_cmdlist);
> 
> I guess you could keep using xstrprintf for these to avoid the extra
> copy...  but what you have LGTM either way.

I've taken your advice.  Below is what I pushed.

Thanks,
Andrew

--

commit 747656685b3e8477868478cd927fbb2834937aff
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Aug 17 13:29:22 2021 +0100

    gdb: make use of std::string in utils.c
    
    Replace some of the manual string management (malloc/free) with
    std::string when creating commands in utils.c.
    
    Things are a little bit messy as, creating the prefix commands (using
    add_basic_prefix_cmd and add_show_prefix_cmd), doesn't copy the doc
    string, while creating the actual set/show commands (using
    add_setshow_enum_cmd) does copy the doc string.
    
    As a result, I have retained the use of xstrprintf when creating the
    prefix command doc strings, but switched to using std::string when
    creating the actual set/show commands.
    
    There should be no user visible changes after this commit.

diff --git a/gdb/utils.c b/gdb/utils.c
index 0009cb10d87..0a7c270b40d 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -508,19 +508,21 @@ add_internal_problem_command (struct internal_problem *problem)
 {
   struct cmd_list_element **set_cmd_list;
   struct cmd_list_element **show_cmd_list;
-  char *set_doc;
-  char *show_doc;
 
   set_cmd_list = XNEW (struct cmd_list_element *);
   show_cmd_list = XNEW (struct cmd_list_element *);
   *set_cmd_list = NULL;
   *show_cmd_list = NULL;
 
-  set_doc = xstrprintf (_("Configure what GDB does when %s is detected."),
-			problem->name);
-
-  show_doc = xstrprintf (_("Show what GDB does when %s is detected."),
-			 problem->name);
+  /* The add_basic_prefix_cmd and add_show_prefix_cmd functions take
+     ownership of the string passed in, which is why we don't need to free
+     set_doc and show_doc in this function.  */
+  const char *set_doc
+    = xstrprintf (_("Configure what GDB does when %s is detected."),
+		  problem->name);
+  const char *show_doc
+    = xstrprintf (_("Show what GDB does when %s is detected."),
+		  problem->name);
 
   add_basic_prefix_cmd (problem->name, class_maintenance, set_doc,
 			set_cmd_list,
@@ -532,48 +534,42 @@ add_internal_problem_command (struct internal_problem *problem)
 
   if (problem->user_settable_should_quit)
     {
-      set_doc = xstrprintf (_("Set whether GDB should quit "
-			      "when an %s is detected."),
-			    problem->name);
-      show_doc = xstrprintf (_("Show whether GDB will quit "
-			       "when an %s is detected."),
-			     problem->name);
+      std::string set_quit_doc
+	= string_printf (_("Set whether GDB should quit when an %s is "
+			   "detected."), problem->name);
+      std::string show_quit_doc
+	= string_printf (_("Show whether GDB will quit when an %s is "
+			   "detected."), problem->name);
       add_setshow_enum_cmd ("quit", class_maintenance,
 			    internal_problem_modes,
 			    &problem->should_quit,
-			    set_doc,
-			    show_doc,
+			    set_quit_doc.c_str (),
+			    show_quit_doc.c_str (),
 			    NULL, /* help_doc */
 			    NULL, /* setfunc */
 			    NULL, /* showfunc */
 			    set_cmd_list,
 			    show_cmd_list);
-
-      xfree (set_doc);
-      xfree (show_doc);
     }
 
   if (problem->user_settable_should_dump_core)
     {
-      set_doc = xstrprintf (_("Set whether GDB should create a core "
-			      "file of GDB when %s is detected."),
-			    problem->name);
-      show_doc = xstrprintf (_("Show whether GDB will create a core "
-			       "file of GDB when %s is detected."),
-			     problem->name);
+      std::string set_core_doc
+	= string_printf (_("Set whether GDB should create a core file of "
+			   "GDB when %s is detected."), problem->name);
+      std::string show_core_doc
+	= string_printf (_("Show whether GDB will create a core file of "
+			   "GDB when %s is detected."), problem->name);
       add_setshow_enum_cmd ("corefile", class_maintenance,
 			    internal_problem_modes,
 			    &problem->should_dump_core,
-			    set_doc,
-			    show_doc,
+			    set_core_doc.c_str (),
+			    show_core_doc.c_str (),
 			    NULL, /* help_doc */
 			    NULL, /* setfunc */
 			    NULL, /* showfunc */
 			    set_cmd_list,
 			    show_cmd_list);
-
-      xfree (set_doc);
-      xfree (show_doc);
     }
 }