diff mbox

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

Message ID 3ff7dd5c-334b-3bcc-e43e-a350b3008304@redhat.com
State New
Headers show

Commit Message

Pedro Alves July 1, 2019, 1:17 p.m. UTC 
On 7/1/19 1:55 PM, Pedro Alves wrote:

> 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.
> 
> - See about implementing support for some ui field type other than
>   integers, to make sure that we're not missing something.
Here's a patch for thing #2.  It adds a string_field type.

It applies on top of the other patch that gets rid of .ptr(), but
it's super easy to make string_field a ctor, that was not the
point here.

I looked for a few things here and there to convert, one thing 
was interesting, the field_fmt() calls.  If we wanted to add some
fmt_field type for that, one which saves a copy of the va_list
as a field, to avoid formatting into a temporary string, then
the "get rid of .ptr()" patch conflicts with that desire, as that
relies on a default argument to int_field/string_field etc, which
doesn't work when you also want a "..." parameter, like:

 static inline string_field_s *
 fmt_field (const char *name, const char *str,
            fmt_s &&tmp = {}, ...)
 {
   tmp.vargs = va_copy ....;
   ...
 }

I'm not sure whether we could portably save the varargs like
above, though.  So that might be moot.

The patch replaces the format_fmt calls with string_printf + string_field.

From ebcc5fb83fb0c6e98db2501c5868ba8cee6dd1f8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 7 Jun 2019 22:45:09 +0100
Subject: [PATCH] string_field

---
 gdb/breakpoint.c | 14 ++++++--------
 gdb/infrun.c     | 19 +++++++------------
 gdb/ui-out.c     | 18 ++++++++++++++++--
 gdb/ui-out.h     | 37 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 65 insertions(+), 23 deletions(-)

Comments

Pedro Alves July 1, 2019, 1:20 p.m. UTC | #1 
On 7/1/19 2:17 PM, Pedro Alves wrote:
> On 7/1/19 1:55 PM, Pedro Alves wrote:
> 
>> 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.
>>
>> - See about implementing support for some ui field type other than
>>   integers, to make sure that we're not missing something.
> Here's a patch for thing #2.  It adds a string_field type.

I pushed the 4 patches I shown today to the users/palves/format_strings-experiment
branch now.  (left users/palves/format_strings untouched.)

Thanks,
Pedro Alves
Tom Tromey July 1, 2019, 5:38 p.m. UTC | #2 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'm not sure whether we could portably save the varargs like
Pedro> above, though.  So that might be moot.

I'm not sure it matters a whole lot.

I looked through all the field_fmt calls, and many of them can be
replaced with some other call.

The few that remain can be rewritten in terms of string_printf if need
be.

Pedro> -      uiout->text ("[Inferior ");
Pedro> -      uiout->text (plongest (inf->num));
Pedro> -      uiout->text (" (");
Pedro> -      uiout->text (pidstr.c_str ());
Pedro> -      uiout->text (") exited with code ");
Pedro> -      uiout->field_fmt ("exit-code", "0%o", (unsigned int) exitstatus);
Pedro> -      uiout->text ("]\n");
Pedro> +      std::string exit_code_str
Pedro> +	= string_printf ("0%o", (unsigned int) exitstatus);
Pedro> +      uiout->message ("[Inferior %s (%s) exited with code %pF]\n",
Pedro> +		      plongest (inf->num), pidstr.c_str (),
Pedro> +		      string_field ("exit-code", exit_code_str.c_str ()));

This is so much better.

Tom
Tom Tromey July 1, 2019, 6:49 p.m. UTC | #3 
Tom> I looked through all the field_fmt calls, and many of them can be
Tom> replaced with some other call.

I wrote some patches to do this where it is possible.

Tom
Pedro Alves July 1, 2019, 6:56 p.m. UTC | #4 
On 7/1/19 7:49 PM, Tom Tromey wrote:
> Tom> I looked through all the field_fmt calls, and many of them can be
> Tom> replaced with some other call.
> 
> I wrote some patches to do this where it is possible.

Awesome.  I wrote a patch to implement %p[ / %p], and am now writing
a patch to document the formatters.

Thanks,
Pedro Alves
Pedro Alves July 1, 2019, 7:25 p.m. UTC | #5 
On 7/1/19 6:38 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I'm not sure whether we could portably save the varargs like
> Pedro> above, though.  So that might be moot.
> 
> I'm not sure it matters a whole lot.
> 
> I looked through all the field_fmt calls, and many of them can be
> replaced with some other call.
> 
> The few that remain can be rewritten in terms of string_printf if need
> be.
> 
> Pedro> -      uiout->text ("[Inferior ");
> Pedro> -      uiout->text (plongest (inf->num));
> Pedro> -      uiout->text (" (");
> Pedro> -      uiout->text (pidstr.c_str ());
> Pedro> -      uiout->text (") exited with code ");
> Pedro> -      uiout->field_fmt ("exit-code", "0%o", (unsigned int) exitstatus);
> Pedro> -      uiout->text ("]\n");
> Pedro> +      std::string exit_code_str
> Pedro> +	= string_printf ("0%o", (unsigned int) exitstatus);
> Pedro> +      uiout->message ("[Inferior %s (%s) exited with code %pF]\n",
> Pedro> +		      plongest (inf->num), pidstr.c_str (),
> Pedro> +		      string_field ("exit-code", exit_code_str.c_str ()));
> 
> This is so much better.

I merged this one to users/palves/format_strings too.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 77f416eb9ca..e2adb18a9fa 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6193,10 +6193,9 @@  print_one_breakpoint_location (struct breakpoint *b,
 	  && breakpoint_condition_evaluation_mode ()
 	  == condition_evaluation_target)
 	{
-	  uiout->text (" (");
-	  uiout->field_string ("evaluated-by",
-			       bp_condition_evaluator (b));
-	  uiout->text (" evals)");
+	  uiout->message (" (%pF evals)",
+			  string_field ("evaluated-by",
+					bp_condition_evaluator (b)));
 	}
       uiout->text ("\n");
     }
@@ -12752,10 +12751,9 @@  tracepoint_print_one_detail (const struct breakpoint *self,
     {
       gdb_assert (self->type == bp_static_tracepoint);
 
-      uiout->text ("\tmarker id is ");
-      uiout->field_string ("static-tracepoint-marker-string-id",
-			   tp->static_trace_marker_id);
-      uiout->text ("\n");
+      uiout->message ("\tmarker id is %pF\n",
+		      string_field ("static-tracepoint-marker-string-id",
+				    tp->static_trace_marker_id));
     }
 }
 
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4fd92f1bac2..604015e1ef8 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7641,24 +7641,19 @@  print_exited_reason (struct ui_out *uiout, int exitstatus)
     {
       if (uiout->is_mi_like_p ())
 	uiout->field_string ("reason", async_reason_lookup (EXEC_ASYNC_EXITED));
-      uiout->text ("[Inferior ");
-      uiout->text (plongest (inf->num));
-      uiout->text (" (");
-      uiout->text (pidstr.c_str ());
-      uiout->text (") exited with code ");
-      uiout->field_fmt ("exit-code", "0%o", (unsigned int) exitstatus);
-      uiout->text ("]\n");
+      std::string exit_code_str
+	= string_printf ("0%o", (unsigned int) exitstatus);
+      uiout->message ("[Inferior %s (%s) exited with code %pF]\n",
+		      plongest (inf->num), pidstr.c_str (),
+		      string_field ("exit-code", exit_code_str.c_str ()));
     }
   else
     {
       if (uiout->is_mi_like_p ())
 	uiout->field_string
 	  ("reason", async_reason_lookup (EXEC_ASYNC_EXITED_NORMALLY));
-      uiout->text ("[Inferior ");
-      uiout->text (plongest (inf->num));
-      uiout->text (" (");
-      uiout->text (pidstr.c_str ());
-      uiout->text (") exited normally]\n");
+      uiout->message ("[Inferior %s (%s) exited normally]\n",
+		      plongest (inf->num), pidstr.c_str ());
     }
 }
 
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 92e461f850e..86bb2f5289a 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -607,8 +607,22 @@  ui_out::vmessage (const char *format, va_list args)
 	    {
 	    case 'F':
 	      {
-		int_field_s *field = va_arg (args, int_field_s *);
-		field_int (field->name, field->val);
+		base_field_s *bf = va_arg (args, base_field_s *);
+		switch (bf->kind)
+		  {
+		  case field_kind::INT:
+		    {
+		      auto *f = (int_field_s *) bf;
+		      field_int (f->name, f->val);
+		    }
+		    break;
+		  case field_kind::STRING:
+		    {
+		      auto *f = (string_field_s *) bf;
+		      field_string (f->name, f->str);
+		    }
+		    break;
+		  }
 	      }
 	      break;
 	    case 's':
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index fbf9c9bd326..e682d44383e 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -68,11 +68,24 @@  enum ui_out_type
     ui_out_type_list
   };
 
+enum class field_kind
+  {
+    INT,
+    STRING,
+  };
+
 /* An int field, to be passed to %pF in format strings.  */
 
-struct int_field_s
+struct base_field_s
 {
   const char *name;
+  field_kind kind;
+};
+
+/* An int field, to be passed to %pF in format strings.  */
+
+struct int_field_s : base_field_s
+{
   int val;
 };
 
@@ -85,10 +98,32 @@  int_field (const char *name, int val,
 	   int_field_s &&tmp = {})
 {
   tmp.name = name;
+  tmp.kind = field_kind::INT;
   tmp.val = val;
   return &tmp;
 }
 
+/* A string field, to be passed to %pF in format strings.  */
+
+struct string_field_s : base_field_s
+{
+  const char *str;
+};
+
+/* Construct a temporary string_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.  */
+
+static inline string_field_s *
+string_field (const char *name, const char *str,
+	      string_field_s &&tmp = {})
+{
+  tmp.name = name;
+  tmp.kind = field_kind::STRING;
+  tmp.str = str;
+  return &tmp;
+}
+
 /* A styled string.  */
 
 struct styled_string_s