diff mbox

[RFA] Change build_address_symbolic to return std::string

Message ID 20180505162454.5619-1-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey May 5, 2018, 4:24 p.m. UTC 
  This changes two out parameters of build_address_symbolic to be
std::string, and updates the callers.  This allows removing some
cleanups.

This patch also moves the declaration of build_address_symbolic out of
defs.h.  I think that many things in defs.h should be elsewhere
instead.  In this case, I moved the declaration to valprint.h, becuase
there is no "printcmd.h" -- but perhaps it would be better to
introduce that instead.

Tested by the buildbot.

gdb/ChangeLog
2018-05-05  Tom Tromey  <tom@tromey.com>

	* valprint.h (build_address_symbolic): Declare.
	* printcmd.c (print_address_symbolic): Update.
	(build_address_symbolic): Change "name" and "filename" to
	std::string.
	* disasm.c (gdb_pretty_print_disassembler::pretty_print_insn):
	Update.
	* defs.h (build_address_symbolic): Remove declaration.
---
 gdb/ChangeLog  | 10 ++++++++++
 gdb/defs.h     |  9 ---------
 gdb/disasm.c   | 11 +++--------
 gdb/printcmd.c | 29 ++++++++++-------------------
 gdb/valprint.h |  9 +++++++++
 5 files changed, 32 insertions(+), 36 deletions(-)

Comments

Tom Tromey May 21, 2018, 4:27 p.m. UTC | #1 
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This changes two out parameters of build_address_symbolic to be
Tom> std::string, and updates the callers.  This allows removing some
Tom> cleanups.

Ping.

Tom
Keith Seitz June 6, 2018, 6:38 p.m. UTC | #2 
Did someone looks this over already?

In any case...

On 05/05/2018 09:24 AM, Tom Tromey wrote:
> This changes two out parameters of build_address_symbolic to be
> std::string, and updates the callers.  This allows removing some
> cleanups.
> 
> This patch also moves the declaration of build_address_symbolic out of
> defs.h.  I think that many things in defs.h should be elsewhere
> instead.  In this case, I moved the declaration to valprint.h, becuase
> there is no "printcmd.h" -- but perhaps it would be better to
> introduce that instead.

That seems reasonable. One small request:

> diff --git a/gdb/valprint.h b/gdb/valprint.h
> index f005c31f87..29053b5028 100644
> --- a/gdb/valprint.h
> +++ b/gdb/valprint.h
> @@ -229,4 +229,13 @@ extern void print_command_parse_format (const char **expp, const char *cmdname,
>  					struct format_data *fmtp);
>  extern void print_value (struct value *val, const struct format_data *fmtp);
>  
> +extern int build_address_symbolic (struct gdbarch *,
> +				   CORE_ADDR addr,
> +				   int do_demangle,
> +				   std::string *name,
> +				   int *offset,
> +				   std::string *filename,
> +				   int *line,
> +				   int *unmapped);
> +
>  #endif

Since you're moving the declaration here, could you please copy the comment from the definition here, too, while you're at it, replacing it with the usual "See XYZ.h"? [Warning: IANAM]

Otherwise, LGTM.

Keith
Tom Tromey June 6, 2018, 10:37 p.m. UTC | #3 
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Did someone looks this over already?

Nope.

>> +extern int build_address_symbolic (struct gdbarch *,
>> +				   CORE_ADDR addr,
>> +				   int do_demangle,
>> +				   std::string *name,
>> +				   int *offset,
>> +				   std::string *filename,
>> +				   int *line,
>> +				   int *unmapped);

Keith> Since you're moving the declaration here, could you please copy
Keith> the comment from the definition here, too, while you're at it,
Keith> replacing it with the usual "See XYZ.h"? [Warning: IANAM]

Good idea, I've made this change locally.

Tom
Simon Marchi June 7, 2018, 3:36 a.m. UTC | #4 
On 2018-06-06 18:37, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> Did someone looks this over already?
> 
> Nope.
> 
>>> +extern int build_address_symbolic (struct gdbarch *,
>>> +				   CORE_ADDR addr,
>>> +				   int do_demangle,
>>> +				   std::string *name,
>>> +				   int *offset,
>>> +				   std::string *filename,
>>> +				   int *line,
>>> +				   int *unmapped);
> 
> Keith> Since you're moving the declaration here, could you please copy
> Keith> the comment from the definition here, too, while you're at it,
> Keith> replacing it with the usual "See XYZ.h"? [Warning: IANAM]
> 
> Good idea, I've made this change locally.
> 
> Tom

The patch LGTM too, including Keith's comment.

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/defs.h b/gdb/defs.h
index dc38a288c5..4cf83f0d44 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -327,15 +327,6 @@  extern int print_address_symbolic (struct gdbarch *, CORE_ADDR,
 				   struct ui_file *, int,
 				   const char *);
 
-extern int build_address_symbolic (struct gdbarch *,
-				   CORE_ADDR addr,
-				   int do_demangle, 
-				   char **name, 
-				   int *offset, 
-				   char **filename, 
-				   int *line, 	
-				   int *unmapped);
-
 extern void print_address (struct gdbarch *, CORE_ADDR, struct ui_file *);
 extern const char *pc_prefix (CORE_ADDR);
 
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 833341a169..2c207d10cb 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -30,6 +30,7 @@ 
 #include "safe-ctype.h"
 #include <algorithm>
 #include "common/gdb_optional.h"
+#include "valprint.h"
 
 /* Disassemble functions.
    FIXME: We should get rid of all the duplicate code in gdb that does
@@ -199,8 +200,6 @@  gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
   int offset;
   int line;
   int size;
-  char *filename = NULL;
-  char *name = NULL;
   CORE_ADDR pc;
   struct gdbarch *gdbarch = arch ();
 
@@ -237,6 +236,7 @@  gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
       uiout->text (pc_prefix (pc));
     uiout->field_core_addr ("address", gdbarch, pc);
 
+    std::string name, filename;
     if (!build_address_symbolic (gdbarch, pc, 0, &name, &offset, &filename,
 				 &line, &unmapped))
       {
@@ -244,7 +244,7 @@  gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
 	   the future.  */
 	uiout->text (" <");
 	if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
-	  uiout->field_string ("func-name", name);
+	  uiout->field_string ("func-name", name.c_str ());
 	uiout->text ("+");
 	uiout->field_int ("offset", offset);
 	uiout->text (">:\t");
@@ -252,11 +252,6 @@  gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
     else
       uiout->text (":\t");
 
-    if (filename != NULL)
-      xfree (filename);
-    if (name != NULL)
-      xfree (name);
-
     m_insn_stb.clear ();
 
     if (flags & DISASSEMBLY_RAW_INSN)
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 4696373b2c..3b9b99ad66 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -520,47 +520,38 @@  print_address_symbolic (struct gdbarch *gdbarch, CORE_ADDR addr,
 			struct ui_file *stream,
 			int do_demangle, const char *leadin)
 {
-  char *name = NULL;
-  char *filename = NULL;
+  std::string name, filename;
   int unmapped = 0;
   int offset = 0;
   int line = 0;
 
-  /* Throw away both name and filename.  */
-  struct cleanup *cleanup_chain = make_cleanup (free_current_contents, &name);
-  make_cleanup (free_current_contents, &filename);
-
   if (build_address_symbolic (gdbarch, addr, do_demangle, &name, &offset,
 			      &filename, &line, &unmapped))
-    {
-      do_cleanups (cleanup_chain);
-      return 0;
-    }
+    return 0;
 
   fputs_filtered (leadin, stream);
   if (unmapped)
     fputs_filtered ("<*", stream);
   else
     fputs_filtered ("<", stream);
-  fputs_filtered (name, stream);
+  fputs_filtered (name.c_str (), stream);
   if (offset != 0)
     fprintf_filtered (stream, "+%u", (unsigned int) offset);
 
   /* Append source filename and line number if desired.  Give specific
      line # of this addr, if we have it; else line # of the nearest symbol.  */
-  if (print_symbol_filename && filename != NULL)
+  if (print_symbol_filename && !filename.empty ())
     {
       if (line != -1)
-	fprintf_filtered (stream, " at %s:%d", filename, line);
+	fprintf_filtered (stream, " at %s:%d", filename.c_str (), line);
       else
-	fprintf_filtered (stream, " in %s", filename);
+	fprintf_filtered (stream, " in %s", filename.c_str ());
     }
   if (unmapped)
     fputs_filtered ("*>", stream);
   else
     fputs_filtered (">", stream);
 
-  do_cleanups (cleanup_chain);
   return 1;
 }
 
@@ -574,9 +565,9 @@  int
 build_address_symbolic (struct gdbarch *gdbarch,
 			CORE_ADDR addr,  /* IN */
 			int do_demangle, /* IN */
-			char **name,     /* OUT */
+			std::string *name, /* OUT */
 			int *offset,     /* OUT */
-			char **filename, /* OUT */
+			std::string *filename, /* OUT */
 			int *line,       /* OUT */
 			int *unmapped)   /* OUT */
 {
@@ -678,7 +669,7 @@  build_address_symbolic (struct gdbarch *gdbarch,
 
   *offset = addr - name_location;
 
-  *name = xstrdup (name_temp);
+  *name = name_temp;
 
   if (print_symbol_filename)
     {
@@ -688,7 +679,7 @@  build_address_symbolic (struct gdbarch *gdbarch,
 
       if (sal.symtab)
 	{
-	  *filename = xstrdup (symtab_to_filename_for_display (sal.symtab));
+	  *filename = symtab_to_filename_for_display (sal.symtab);
 	  *line = sal.line;
 	}
     }
diff --git a/gdb/valprint.h b/gdb/valprint.h
index f005c31f87..29053b5028 100644
--- a/gdb/valprint.h
+++ b/gdb/valprint.h
@@ -229,4 +229,13 @@  extern void print_command_parse_format (const char **expp, const char *cmdname,
 					struct format_data *fmtp);
 extern void print_value (struct value *val, const struct format_data *fmtp);
 
+extern int build_address_symbolic (struct gdbarch *,
+				   CORE_ADDR addr,
+				   int do_demangle,
+				   std::string *name,
+				   int *offset,
+				   std::string *filename,
+				   int *line,
+				   int *unmapped);
+
 #endif