Re: ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output)

[Pedro Alves <palves@redhat.com>]

On 7/1/19 1:55 PM, Pedro Alves wrote:
> On 7/1/19 1:23 PM, Pedro Alves wrote:
>>> What's left to do on that branch in order to merge it?
>> Well, you used printf_filtered the new patches, but that doesn't work
>> with the new format specifiers, yet.  :-)
> 
> There are two other things that I wanted to experiment/try first.
> 
> - See if we could get rid of the need for the ".ptr ()" calls.

So those "ptr ()" calls/members are there because int_field/styled_string
are ctors, and, we can't pass references via va_list.  We can get rid of those,
if we replace the ctors with functions that allocate a temporary on the
callers stack, like:

 static inline styled_string_s *
 styled_string (const ui_file_style &style, const char *str,
               styled_string_s &&tmp = {})
 {
   tmp.style = style;
   tmp.str = str;
   return &tmp;
 }

Actually, what I originally thought to try, was to make it
as short as possible to write that "styled_string" expression.
I even experimented with renaming that styled_string function
above to "ss", resulting in:

  printf_filtered (": file %ps, line %d.",
                   ss (file_name_style.style (), filename),
                   b->loc->line_number);

but I reverted it, thinking that I was maybe overdoing it.

We'll still need to use .ptr() with:

 	  fprintf_filtered (stream, " %pS<repeats %u times>%pS",
                             metadata_style.style ().ptr (), reps, nullptr);

(whatever the formatter spelling we end up with)

unless we do make style() return a pointer.

Here's a patch implementing the idea above.  I wrote this a couple
weeks ago, and at the time I felt more strongly about it.  Is this
worth it?

From fd5bce2ea4716552ce9c3f9fdac7628cbf5a5cd2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 7 Jun 2019 22:42:57 +0100
Subject: [PATCH] down with .ptr()

---
 gdb/breakpoint.c | 14 ++++++------
 gdb/macrocmd.c   |  2 +-
 gdb/printcmd.c   |  2 +-
 gdb/symfile.c    |  8 +++----
 gdb/symtab.c     |  6 +++---
 gdb/ui-out.c     |  8 +++----
 gdb/ui-out.h     | 65 ++++++++++++++++++++++++++++++--------------------------
 7 files changed, 54 insertions(+), 51 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 14ff32a68a0..77f416eb9ca 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4930,7 +4930,7 @@ watchpoint_check (bpstat bs)
 	  uiout->message ("\nWatchpoint %pF deleted because the program has "
 			  "left the block in\n"
 			  "which its expression is valid.\n",
-			  int_field ("wpnum", b->number).ptr ());
+			  int_field ("wpnum", b->number));
 	}
 
       /* Make sure the watchpoint's commands aren't executed.  */
@@ -6246,7 +6246,7 @@ print_one_breakpoint_location (struct breakpoint *b,
     {
       annotate_field (8);
       uiout->message ("\tignore next %pF hits\n",
-		      int_field ("ignore", b->ignore_count).ptr ());
+		      int_field ("ignore", b->ignore_count));
     }
 
   /* Note that an enable count of 1 corresponds to "enable once"
@@ -6695,7 +6695,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 	  }
       current_uiout->message (_("also set at pc %ps.\n"),
 			      styled_string (address_style.style (),
-					     paddress (gdbarch, pc)).ptr ());
+					     paddress (gdbarch, pc)));
     }
 }
 \f
@@ -12115,7 +12115,7 @@ say_where (struct breakpoint *b)
 	printf_filtered (" at %ps",
 			 styled_string (address_style.style (),
 					paddress (b->loc->gdbarch,
-						  b->loc->address)).ptr ());
+						  b->loc->address)));
       if (b->loc->symtab != NULL)
 	{
 	  /* If there is a single location, we can print the location
@@ -12126,7 +12126,7 @@ say_where (struct breakpoint *b)
 		= symtab_to_filename_for_display (b->loc->symtab);
 	      printf_filtered (": file %ps, line %d.",
 			       styled_string (file_name_style.style (),
-					      filename).ptr (),
+					      filename),
 			       b->loc->line_number);
 	    }
 	  else
@@ -12433,10 +12433,10 @@ bkpt_print_it (bpstat bs)
     }
   if (bp_temp)
     uiout->message ("Temporary breakpoint %pF, ",
-		    int_field ("bkptno", b->number).ptr ());
+		    int_field ("bkptno", b->number));
   else
     uiout->message ("Breakpoint %pF, ",
-		    int_field ("bkptno", b->number).ptr ());
+		    int_field ("bkptno", b->number));
 
   return PRINT_SRC_AND_LOC;
 }
diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 0f1f239c5a5..cb7267d07a3 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -122,7 +122,7 @@ show_pp_source_pos (struct ui_file *stream,
   std::string fullname = macro_source_fullname (file);
   fprintf_filtered (stream, "%ps:%d\n",
 		    styled_string (file_name_style.style (),
-				   fullname.c_str ()).ptr (),
+				   fullname.c_str ()),
 		    line);
 
   while (file->included_by)
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 0aa9ac07819..61deb37c349 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2190,7 +2190,7 @@ print_variable_and_value (const char *name, struct symbol *var,
     name = SYMBOL_PRINT_NAME (var);
 
   fprintf_filtered (stream, "%s%ps = ", n_spaces (2 * indent),
-		    styled_string (variable_name_style.style (), name).ptr ());
+		    styled_string (variable_name_style.style (), name));
 
   try
     {
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 903f2ff817b..03f9013315c 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1117,7 +1117,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
       else
 	current_uiout->message (_("Reading symbols from %ps...\n"),
 				styled_string (file_name_style.style (),
-					       name).ptr ());
+					       name));
     }
   syms_from_objfile (objfile, addrs, add_flags);
 
@@ -1130,8 +1130,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
     {
       if (should_print)
 	current_uiout->message (_("Expanding full symbols from %ps...\n"),
-				styled_string (file_name_style.style (),
-					       name).ptr ());
+				styled_string (file_name_style.style (), name));
 
       if (objfile->sf)
 	objfile->sf->qf->expand_all_symtabs (objfile);
@@ -1144,8 +1143,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
   if (should_print && !objfile_has_symbols (objfile)
       && objfile->separate_debug_objfile == nullptr)
     current_uiout->message (_("(No debugging symbols found in %ps)\n"),
-			    styled_string (file_name_style.style (),
-					   name).ptr ());
+			    styled_string (file_name_style.style (), name));
 
   if (should_print)
     {
diff --git a/gdb/symtab.c b/gdb/symtab.c
index e5f702ab804..e6253604c5f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4605,7 +4605,7 @@ print_symbol_info (enum search_domain kind,
 	{
 	  current_uiout->message
 	    (_("\nFile %ps:\n"),
-	     styled_string (file_name_style.style (), s_filename).ptr ());
+	     styled_string (file_name_style.style (), s_filename));
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4658,8 +4658,8 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
 
   current_uiout->message
     (_("%ps  %ps\n"),
-     styled_string (address_style.style (), tmp).ptr (),
-     styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ());
+     styled_string (address_style.style (), tmp),
+     styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)));
 }
 
 /* This is the guts of the commands "info functions", "info types", and
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 25efa98d56a..92e461f850e 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -607,14 +607,14 @@ ui_out::vmessage (const char *format, va_list args)
 	    {
 	    case 'F':
 	      {
-		int_field *field = va_arg (args, int_field *);
-		field_int (field->name (), field->val ());
+		int_field_s *field = va_arg (args, int_field_s *);
+		field_int (field->name, field->val);
 	      }
 	      break;
 	    case 's':
 	      {
-		styled_string *ss = va_arg (args, styled_string *);
-		call_do_message (ss->style (), "%s", ss->str ());
+		styled_string_s *ss = va_arg (args, styled_string_s *);
+		call_do_message (ss->style, "%s", ss->str);
 	      }
 	      break;
 	    case 'S':
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 35bc8b21824..fbf9c9bd326 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -68,45 +68,50 @@ enum ui_out_type
     ui_out_type_list
   };
 
-struct int_field
+/* An int field, to be passed to %pF in format strings.  */
+
+struct int_field_s
 {
-  int_field (const char *name, int val)
-    : m_name (name),
-      m_val (val)
-  {
-  }
+  const char *name;
+  int val;
+};
 
-  /* We need this because we can't pass a reference via
-     va_args.  */
-  const int_field *ptr () const { return this; }
+/* Construct a temporary int_field_s on the caller's stack and return
+   a pointer to the constructed object.  We use this because it's not
+   possible to pass a reference via va_args.  */
 
-  const char *name () const {return m_name; }
-  int val () const {return m_val; }
+static inline int_field_s *
+int_field (const char *name, int val,
+	   int_field_s &&tmp = {})
+{
+  tmp.name = name;
+  tmp.val = val;
+  return &tmp;
+}
 
-private:
-  const char *m_name;
-  int m_val;
-};
+/* A styled string.  */
 
-struct styled_string
+struct styled_string_s
 {
-  styled_string (const ui_file_style &style, const char *str)
-    : m_style (style),
-      m_str (str)
-  {
-  }
+  /* The style.  */
+  ui_file_style style;
 
-  /* We need this because we can't pass a reference via
-     va_args.  */
-  const styled_string *ptr () const { return this; }
+  /* The string.  */
+  const char *str;
+};
 
-  const ui_file_style &style () const { return m_style; }
-  const char *str () const {return m_str; }
+/* Construct a temporary styled_string_s on the caller's stack and
+   return a pointer to the constructed object.  We use this because
+   it's not possible to pass a reference via va_args.  */
 
-private:
-  ui_file_style m_style;
-  const char *m_str;
-};
+static inline styled_string_s *
+styled_string (const ui_file_style &style, const char *str,
+	       styled_string_s &&tmp = {})
+{
+  tmp.style = style;
+  tmp.str = str;
+  return &tmp;
+}
 
 class ui_out
 {
-- 
2.14.5