[PATCH] Style "pwd" output

[PATCH] Style "pwd" output

[Tom Tromey]

This changes the "pwd" command to style its output.
Tested on x86-64 Fedora 29.

gdb/ChangeLog
2019-06-04  Tom Tromey  <tom@tromey.com>

	* cli/cli-cmds.c (pwd_command): Style output.

gdb/testsuite/ChangeLog
2019-06-04  Tom Tromey  <tom@tromey.com>

	* gdb.base/style.exp: Test "pwd".
---
 gdb/ChangeLog                    |  4 ++++
 gdb/cli/cli-cmds.c               | 14 +++++++++-----
 gdb/testsuite/ChangeLog          |  4 ++++
 gdb/testsuite/gdb.base/style.exp |  2 ++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 09f932c2d21..658b08e49a6 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -49,6 +49,7 @@
 #include "cli/cli-script.h"
 #include "cli/cli-setshow.h"
 #include "cli/cli-cmds.h"
+#include "cli/cli-style.h"
 #include "cli/cli-utils.h"
 
 #include "extension.h"
@@ -337,11 +338,14 @@ pwd_command (const char *args, int from_tty)
     error (_("Error finding name of working directory: %s"),
            safe_strerror (errno));
 
-  if (strcmp (cwd.get (), current_directory) != 0)
-    printf_unfiltered (_("Working directory %s\n (canonically %s).\n"),
-		       current_directory, cwd.get ());
-  else
-    printf_unfiltered (_("Working directory %s.\n"), current_directory);
+  fputs_filtered (_("Working directory "), gdb_stdout);
+  fputs_styled (current_directory, file_name_style.style (), gdb_stdout);
+  if (strcmp (cwd.get (), current_directory) == 0)
+    {
+      fputs_filtered (_("\n (canonically "), gdb_stdout);
+      fputs_styled (cwd.get (), file_name_style.style (), gdb_stdout);
+    }
+  fputs_filtered (".\n", gdb_stdout);
 }
 
 void
diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index a17f2014865..0b2d52a9dea 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -127,4 +127,6 @@ save_vars { env(TERM) } {
     gdb_test "file $binfile" \
 	"Reading symbols from [style $quoted file]..." \
 	"filename is styled when loading symbol file"
+
+    gdb_test "pwd" "Working directory [style .*? file].*"
 }
-- 
2.17.2

Re: [PATCH] Style "pwd" output

[Pedro Alves]

On 6/5/19 3:01 AM, Tom Tromey wrote:
> This changes the "pwd" command to style its output.
> Tested on x86-64 Fedora 29.
> 
> gdb/ChangeLog
> 2019-06-04  Tom Tromey  <tom@tromey.com>
> 
> 	* cli/cli-cmds.c (pwd_command): Style output.
> 
> gdb/testsuite/ChangeLog
> 2019-06-04  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.base/style.exp: Test "pwd".
> ---
>  gdb/ChangeLog                    |  4 ++++
>  gdb/cli/cli-cmds.c               | 14 +++++++++-----
>  gdb/testsuite/ChangeLog          |  4 ++++
>  gdb/testsuite/gdb.base/style.exp |  2 ++
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 09f932c2d21..658b08e49a6 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -49,6 +49,7 @@
>  #include "cli/cli-script.h"
>  #include "cli/cli-setshow.h"
>  #include "cli/cli-cmds.h"
> +#include "cli/cli-style.h"
>  #include "cli/cli-utils.h"
>  
>  #include "extension.h"
> @@ -337,11 +338,14 @@ pwd_command (const char *args, int from_tty)
>      error (_("Error finding name of working directory: %s"),
>             safe_strerror (errno));
>  
> -  if (strcmp (cwd.get (), current_directory) != 0)
> -    printf_unfiltered (_("Working directory %s\n (canonically %s).\n"),
> -		       current_directory, cwd.get ());
> -  else
> -    printf_unfiltered (_("Working directory %s.\n"), current_directory);
> +  fputs_filtered (_("Working directory "), gdb_stdout);
> +  fputs_styled (current_directory, file_name_style.style (), gdb_stdout);
> +  if (strcmp (cwd.get (), current_directory) == 0)
> +    {
> +      fputs_filtered (_("\n (canonically "), gdb_stdout);
> +      fputs_styled (cwd.get (), file_name_style.style (), gdb_stdout);
> +    }
> +  fputs_filtered (".\n", gdb_stdout);

Seems fine to me.

I wish we didn't have to split the lines across different calls though.
I know we don't officially do i18n yet (*), but these kinds of changes will
only make it more difficult to get there.

Maybe with something like:

  if (strcmp (cwd.get (), current_directory) != 0)
    printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),
		       current_directory, cwd.get ());
  else
    printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);

I'm sure you've considered something like that; I think we've discussed it
before.  What are your current thoughts?

(*) - I don't think there's any real blocker other than someone
      figuring out the process.  We should just do with that binutils
      folks do, I suppose.

Thanks,
Pedro Alves

Re: [PATCH] Style "pwd" output

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I wish we didn't have to split the lines across different calls though.
Pedro> I know we don't officially do i18n yet (*), but these kinds of changes will
Pedro> only make it more difficult to get there.

Pedro> Maybe with something like:

Pedro>   if (strcmp (cwd.get (), current_directory) != 0)
Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),
Pedro> 		       current_directory, cwd.get ());
Pedro>   else
Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);

Pedro> I'm sure you've considered something like that; I think we've discussed it
Pedro> before.  What are your current thoughts?

I think what would be nice is a gdb-specific printf extension for this,
e.g.:

    printf_unfiltered ("Working directory %<%s%>\n",
                       ui_out_style_kind::FILE, 
                       current_directory);

%< takes a style argument and %> does not.

However, for that to work, we'd need a GCC enhancement to let gdb modify
the printf format checking.


Note that if we want to be serious about i18n then there is also the
problem that the whole MI approach is not very i18n-friendly.  There are
spots that split calls like this, but which can't easily be unsplit
(even with a printf extension) due to MI.  So maybe something bigger is
needed.

Tom

Re: [PATCH] Style "pwd" output

[Pedro Alves]

On 6/5/19 2:42 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I wish we didn't have to split the lines across different calls though.
> Pedro> I know we don't officially do i18n yet (*), but these kinds of changes will
> Pedro> only make it more difficult to get there.
> 
> Pedro> Maybe with something like:
> 
> Pedro>   if (strcmp (cwd.get (), current_directory) != 0)
> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),
> Pedro> 		       current_directory, cwd.get ());
> Pedro>   else
> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);
> 
> Pedro> I'm sure you've considered something like that; I think we've discussed it
> Pedro> before.  What are your current thoughts?
> 
> I think what would be nice is a gdb-specific printf extension for this,
> e.g.:
> 
>     printf_unfiltered ("Working directory %<%s%>\n",
>                        ui_out_style_kind::FILE, 
>                        current_directory);
> 
> %< takes a style argument and %> does not.
> 
> However, for that to work, we'd need a GCC enhancement to let gdb modify
> the printf format checking.

While not so good looking, instead of a GCC enhancement, we could maybe
do it the Linux way, like binutils did too:

 https://sourceware.org/ml/binutils/2018-02/msg00306.html

See 871b3ab29e87c.

So e.g., %pS would be a style, a %pN would be no style.

    printf_unfiltered ("Working directory %pS%s%pN\n",
                       ui_out_style_kind::FILE, 
                       current_directory);

> 
> 
> Note that if we want to be serious about i18n then there is also the
> problem that the whole MI approach is not very i18n-friendly.  There are
> spots that split calls like this, but which can't easily be unsplit
> (even with a printf extension) due to MI.  So maybe something bigger is
> needed.

Right.

So instead of e.g.:

 uiout->text ("\nWatchpoint ");
 uiout->field_int ("wpnum", b->number);
 uiout->text (" deleted because the program has left the block in\n"
              "which its expression is valid.\n");

We could have:

 uiout->field_fmt ("\nWatchpoint %pF deleted because the program "
                    "has left the block in\n"
                    "which its expression is valid.\n"", 
                    int_field ("wpnum", b->number).ptr ());

With ui_out_field being something like 

 struct int_field
 {
    int_field (const char *field_name, int val);

    // need this because we can't pass a reference via va_args.
    void *ptr () { return this; }

    const char *m_field_name;
    int m_val;
 };

There would then be another class for CORE_ADDR, another for strings,
etc, matching the ui_out::field_int, ui_out::field_string, etc. methods.
Or alternatively, we could have a single uiout_field class that records 
its type depending on which ctor was called.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 6/5/19 4:21 PM, Pedro Alves wrote:
> On 6/5/19 2:42 PM, Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> I wish we didn't have to split the lines across different calls though.
>> Pedro> I know we don't officially do i18n yet (*), but these kinds of changes will
>> Pedro> only make it more difficult to get there.
>>
>> Pedro> Maybe with something like:
>>
>> Pedro>   if (strcmp (cwd.get (), current_directory) != 0)
>> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>\n (canonically <style=filename>%s<style/>).\n"),
>> Pedro> 		       current_directory, cwd.get ());
>> Pedro>   else
>> Pedro>     printf_unfiltered (_("Working directory <style=filename>%s<style/>.\n"), current_directory);
>>
>> Pedro> I'm sure you've considered something like that; I think we've discussed it
>> Pedro> before.  What are your current thoughts?
>>
>> I think what would be nice is a gdb-specific printf extension for this,
>> e.g.:
>>
>>     printf_unfiltered ("Working directory %<%s%>\n",
>>                        ui_out_style_kind::FILE, 
>>                        current_directory);
>>
>> %< takes a style argument and %> does not.
>>
>> However, for that to work, we'd need a GCC enhancement to let gdb modify
>> the printf format checking.
> 
> While not so good looking, instead of a GCC enhancement, we could maybe
> do it the Linux way, like binutils did too:
> 
>  https://sourceware.org/ml/binutils/2018-02/msg00306.html
> 
> See 871b3ab29e87c.
> 
> So e.g., %pS would be a style, a %pN would be no style.
> 
>     printf_unfiltered ("Working directory %pS%s%pN\n",
>                        ui_out_style_kind::FILE, 
>                        current_directory);
> 
>>
>>
>> Note that if we want to be serious about i18n then there is also the
>> problem that the whole MI approach is not very i18n-friendly.  There are
>> spots that split calls like this, but which can't easily be unsplit
>> (even with a printf extension) due to MI.  So maybe something bigger is
>> needed.
> 
> Right.
> 
> So instead of e.g.:
> 
>  uiout->text ("\nWatchpoint ");
>  uiout->field_int ("wpnum", b->number);
>  uiout->text (" deleted because the program has left the block in\n"
>               "which its expression is valid.\n");
> 
> We could have:
> 
>  uiout->field_fmt ("\nWatchpoint %pF deleted because the program "
>                     "has left the block in\n"
>                     "which its expression is valid.\n"", 
>                     int_field ("wpnum", b->number).ptr ());
> 
> With ui_out_field being something like 
> 
>  struct int_field
>  {
>     int_field (const char *field_name, int val);
> 
>     // need this because we can't pass a reference via va_args.
>     void *ptr () { return this; }
> 
>     const char *m_field_name;
>     int m_val;
>  };
> 
> There would then be another class for CORE_ADDR, another for strings,
> etc, matching the ui_out::field_int, ui_out::field_string, etc. methods.
> Or alternatively, we could have a single uiout_field class that records 
> its type depending on which ctor was called.
> 

Prototype time!

So I looked around a bit how to prototype this, and borrowed format_pieces,
and some code from printf_command.

I hooked up "%pF" for ui_out fields, and added that int_field struct above.

I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
Is there a global current style stack we can push a style to/from?

I tested this by loading a program under gdb, and using "start", and
checking that the breakpoint number came out as expected, both in
CLI mode:

 Temporary breakpoint 1, main () at threads.c:57
 57          long i = 0;

and in MI mode:
  *stopped,reason="breakpoint-hit",disp="del",bkptno="1", ....

Pushed to the users/palves/format_strings branch.

From bef62542b4f0f650e65ddcb72611116e73df9269 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Jun 2019 09:17:16 +0100
Subject: [PATCH] Make ui_out::message support %pF, %pS, %pN

---
 gdb/breakpoint.c    | 25 ++++++++--------
 gdb/common/format.c | 15 +++++++++-
 gdb/common/format.h |  2 +-
 gdb/ui-out.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/ui-out.h        | 21 +++++++++++++
 5 files changed, 133 insertions(+), 16 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 08083645e1e..c4270064275 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4927,10 +4927,10 @@ watchpoint_check (bpstat bs)
 	  if (uiout->is_mi_like_p ())
 	    uiout->field_string
 	      ("reason", async_reason_lookup (EXEC_ASYNC_WATCHPOINT_SCOPE));
-	  uiout->text ("\nWatchpoint ");
-	  uiout->field_int ("wpnum", b->number);
-	  uiout->text (" deleted because the program has left the block in\n"
-		       "which its expression is valid.\n");
+	  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 ());
 	}
 
       /* Make sure the watchpoint's commands aren't executed.  */
@@ -6245,9 +6245,8 @@ print_one_breakpoint_location (struct breakpoint *b,
   if (!part_of_multiple && b->ignore_count)
     {
       annotate_field (8);
-      uiout->text ("\tignore next ");
-      uiout->field_int ("ignore", b->ignore_count);
-      uiout->text (" hits\n");
+      uiout->message ("\tignore next %pF hits\n",
+		      int_field ("ignore", b->ignore_count).ptr ());
     }
 
   /* Note that an enable count of 1 corresponds to "enable once"
@@ -12428,18 +12427,18 @@ bkpt_print_it (bpstat bs)
   annotate_breakpoint (b->number);
   maybe_print_thread_hit_breakpoint (uiout);
 
-  if (bp_temp)
-    uiout->text ("Temporary breakpoint ");
-  else
-    uiout->text ("Breakpoint ");
   if (uiout->is_mi_like_p ())
     {
       uiout->field_string ("reason",
 			   async_reason_lookup (EXEC_ASYNC_BREAKPOINT_HIT));
       uiout->field_string ("disp", bpdisp_text (b->disposition));
     }
-  uiout->field_int ("bkptno", b->number);
-  uiout->text (", ");
+  if (bp_temp)
+    uiout->message ("Temporary breakpoint %pF, ",
+		    int_field ("bkptno", b->number).ptr ());
+  else
+    uiout->message ("Breakpoint %pF, ",
+		    int_field ("bkptno", b->number).ptr ());
 
   return PRINT_SRC_AND_LOC;
 }
diff --git a/gdb/common/format.c b/gdb/common/format.c
index fb3421e62bf..d33eab2b2b0 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -20,7 +20,7 @@
 #include "common-defs.h"
 #include "format.h"
 
-format_pieces::format_pieces (const char **arg)
+format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 {
   const char *s;
   char *f, *string;
@@ -251,6 +251,19 @@ format_pieces::format_pieces (const char **arg)
 	      bad = 1;
 	    if (seen_hash || seen_zero || seen_space || seen_plus)
 	      bad = 1;
+
+	    if (gdb_extensions)
+	      {
+		switch (f[1])
+		  {
+		  case 'S':
+		  case 'F':
+		  case 'N':
+		    f++;
+		    break;
+		  }
+	      }
+
 	    break;
 
 	  case 's':
diff --git a/gdb/common/format.h b/gdb/common/format.h
index 058844cec7b..52c0b152bcd 100644
--- a/gdb/common/format.h
+++ b/gdb/common/format.h
@@ -70,7 +70,7 @@ class format_pieces
 {
 public:
 
-  format_pieces (const char **arg);
+  format_pieces (const char **arg, bool gdb_extensions = false);
   ~format_pieces () = default;
 
   DISABLE_COPY_AND_ASSIGN (format_pieces);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 6851fd29c6a..9c987a6cfa6 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -24,6 +24,7 @@
 #include "expression.h"		/* For language.h */
 #include "language.h"
 #include "ui-out.h"
+#include "common/format.h"
 
 #include <vector>
 #include <memory>
@@ -547,7 +548,7 @@ ui_out::text (const char *string)
 }
 
 void
-ui_out::message (const char *format, ...)
+ui_out::call_do_message (const char *format, ...)
 {
   va_list args;
 
@@ -556,6 +557,89 @@ ui_out::message (const char *format, ...)
   va_end (args);
 }
 
+void
+ui_out::message (const char *format, ...)
+{
+  format_pieces fpieces (&format, true);
+
+  va_list args;
+  va_start (args, format);
+
+  for (auto &&piece : fpieces)
+    {
+      const char *current_substring = piece.string;
+
+      switch (piece.argclass)
+	{
+	case string_arg:
+	  call_do_message (current_substring, va_arg (args, const char *));
+	  break;
+	case wide_string_arg:
+	  /* FIXME */
+	  break;
+	case wide_char_arg:
+	  /* FIXME */
+	  break;
+	case long_long_arg:
+#ifdef PRINTF_HAS_LONG_LONG
+	  call_do_message (current_substring, va_arg (args, long long));
+	  break;
+#else
+	  error (_("long long not supported in ui_out::message"));
+#endif
+	case int_arg:
+	  call_do_message (current_substring, va_arg (args, int));
+	  break;
+	case long_arg:
+	  call_do_message (current_substring, va_arg (args, long));
+	  break;
+	  /* Handle floating-point values.  */
+	case double_arg:
+	case long_double_arg:
+	case dec32float_arg:
+	case dec64float_arg:
+	case dec128float_arg:
+	  /* FIXME */
+	  break;
+	case ptr_arg:
+	  switch (current_substring[2])
+	    {
+	    case 'F':
+	      {
+		int_field *field = va_arg (args, int_field *);
+		field_int (field->name (), field->val ());
+	      }
+	      break;
+	    case 'S':
+	      /* Push style on stack?  */
+	      break;
+	    case 'N':
+	      /* Pop style from stack?  */
+	      break;
+	    default:
+	      call_do_message (current_substring, va_arg (args, void *));
+	      break;
+	    }
+	  break;
+	case literal_piece:
+	  /* Print a portion of the format string that has no
+	     directives.  Note that this will not include any ordinary
+	     %-specs, but it might include "%%".  That is why we use
+	     printf_filtered and not puts_filtered here.  Also, we
+	     pass a dummy argument because some platforms have
+	     modified GCC to include -Wformat-security by default,
+	     which will warn here if there is no argument.  */
+	  call_do_message (current_substring, 0);
+	  break;
+	default:
+	  internal_error (__FILE__, __LINE__,
+			  _("failed internal consistency check"));
+	}
+    }
+
+  va_end (args);
+}
+
 void
 ui_out::wrap_hint (const char *identstring)
 {
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9eba70eedac..b20d9508330 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -83,6 +83,26 @@ enum class ui_out_style_kind
   ADDRESS
 };
 
+struct int_field
+{
+  int_field (const char *name, int val)
+    : m_name (name),
+      m_val (val)
+  {
+  }
+
+  /* We need this because we can't pass a reference via
+     va_args.  */
+  const int_field *ptr () const { return this; }
+
+  const char *name () const {return m_name; }
+  int val () const {return m_val; }
+
+private:
+  const char *m_name;
+  int m_val;
+};
+
 class ui_out
 {
  public:
@@ -181,6 +201,7 @@ class ui_out
   { return false; }
 
  private:
+  void call_do_message (const char *format, ...);
 
   ui_out_flags m_flags;
 
-- 
2.14.5

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Prototype time!

Nice.

Pedro> I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
Pedro> Is there a global current style stack we can push a style to/from?

No, there's only a way to set the current style.  However, it's not
really intended for callers to set the style and then just leave it set
-- all the existing calls should set the style, then later set it back
to the default style.

It seems to me that this could be enforced by our printf.  Or, if we
really do want stacking, the stack could just be local to this function.

I wouldn't mind some kind of brackets being used as the characters here,
like %p[ ... %p].  It's too bad that we still have to pass some pointer
value for the closer.

Tom

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

[Pedro Alves]

On 6/5/19 9:27 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Prototype time!
> 
> Nice.
> 
> Pedro> I reserved "%pS" and "%pN" for styling, but haven't really prototyped that.
> Pedro> Is there a global current style stack we can push a style to/from?
> 
> No, there's only a way to set the current style.  However, it's not
> really intended for callers to set the style and then just leave it set
> -- all the existing calls should set the style, then later set it back
> to the default style.
> 
> It seems to me that this could be enforced by our printf.  Or, if we
> really do want stacking, the stack could just be local to this function.
> 
> I wouldn't mind some kind of brackets being used as the characters here,
> like %p[ ... %p].  It's too bad that we still have to pass some pointer
> value for the closer.
Yeah.  I prototyped %pS/%pN now, and having to pass a value for the closer
is ... yeah, a bit ugly.

Also, since gcc expects a pointer for %p, and we want to pass an
enum for the style, I added a small pointer-wrapper hack -- see the
ptr function.  Yay C++.

I've not been paying much attention to the styling patches, so I can't
off hand tell which places would benefit the most from this.  So I just
grepped for _styled and replaced a couple spots.  Likely there are better
examples.

Anyway, see below.

The format strings do get a bit ... wild:

   "%pS%s%pN  %pS%s%pN\n"

With your suggestion, it'd look like:

   "%p[%s%p]  %p[%s%p]\n"

I'm thinking that %pS/%pN %p[/%pN] are more appropriate when
the styled string is constant, like:

   "this %p[text here%p] should be styled\n"

For the seemingly common case of printing a string variable
with a style, I'm thinking that a specific formatter would
be better.  I'll post a follow up patch for that.

From 8f1d8637839fc953d089f4816a12531fbf35202b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Jun 2019 20:44:54 +0100
Subject: [PATCH 1/2] Support %pS / %pN for styling

---
 gdb/breakpoint.c |  6 +++---
 gdb/cli-out.c    | 50 ++++++++++++++++++++++++++------------------------
 gdb/cli-out.h    |  5 +++--
 gdb/mi/mi-out.c  |  3 ++-
 gdb/mi/mi-out.h  |  5 +++--
 gdb/symtab.c     | 26 ++++++++++++++------------
 gdb/ui-out.c     | 23 +++++++++++++----------
 gdb/ui-out.h     | 25 ++++++++++++++++++++++---
 gdb/utils.c      | 10 ++++++++++
 gdb/utils.h      |  6 ++++++
 10 files changed, 102 insertions(+), 57 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c4270064275..0c3c7df92fb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6693,9 +6693,9 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     (others > 1) ? "," 
 			     : ((others == 1) ? " and" : ""));
 	  }
-      printf_filtered (_("also set at pc "));
-      fputs_styled (paddress (gdbarch, pc), address_style.style (), gdb_stdout);
-      printf_filtered (".\n");
+      current_uiout->message (_("also set at pc %pS%s%pN.\n"),
+			      ptr (ui_out_style_kind::ADDRESS),
+			      paddress (gdbarch, pc), nullptr);
     }
 }
 \f
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 55c8d2b3b1b..71ed5c3cf33 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -118,6 +118,27 @@ cli_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
 		   ui_out_style_kind::DEFAULT);
 }
 
+static ui_file_style
+style_from_style_kind (ui_out_style_kind kind)
+{
+  switch (kind)
+    {
+    case ui_out_style_kind::DEFAULT:
+      /* Nothing.  */
+      return {};
+    case ui_out_style_kind::FILE:
+      return file_name_style.style ();
+    case ui_out_style_kind::FUNCTION:
+      return function_name_style.style ();
+    case ui_out_style_kind::VARIABLE:
+      return variable_name_style.style ();
+    case ui_out_style_kind::ADDRESS:
+      return address_style.style ();
+    default:
+      gdb_assert_not_reached ("missing case");
+    }
+}
+
 /* other specific cli_field_* end up here so alignment and field
    separators are both handled by cli_field_string */
 
@@ -160,28 +181,7 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
 
   if (string)
     {
-      ui_file_style fstyle;
-      switch (style)
-	{
-	case ui_out_style_kind::DEFAULT:
-	  /* Nothing.  */
-	  break;
-	case ui_out_style_kind::FILE:
-	  /* Nothing.  */
-	  fstyle = file_name_style.style ();
-	  break;
-	case ui_out_style_kind::FUNCTION:
-	  fstyle = function_name_style.style ();
-	  break;
-	case ui_out_style_kind::VARIABLE:
-	  fstyle = variable_name_style.style ();
-	  break;
-	case ui_out_style_kind::ADDRESS:
-	  fstyle = address_style.style ();
-	  break;
-	default:
-	  gdb_assert_not_reached ("missing case");
-	}
+      ui_file_style fstyle = style_from_style_kind (style);
       fputs_styled (string, fstyle, m_streams.back ());
     }
 
@@ -227,12 +227,14 @@ cli_ui_out::do_text (const char *string)
 }
 
 void
-cli_ui_out::do_message (const char *format, va_list args)
+cli_ui_out::do_message (ui_out_style_kind style,
+			const char *format, va_list args)
 {
   if (m_suppress_output)
     return;
 
-  vfprintf_unfiltered (m_streams.back (), format, args);
+  ui_file_style fstyle = style_from_style_kind (style);
+  vfprintf_styled (m_streams.back (), fstyle, format, args);
 }
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index eeb555fbbec..4ed7b396a57 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -59,8 +59,9 @@ protected:
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (const char *format, va_list args) override
-    ATTRIBUTE_PRINTF (2,0);
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args) override
+    ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index d8bee0f3927..bb52360e540 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -159,7 +159,8 @@ mi_ui_out::do_text (const char *string)
 }
 
 void
-mi_ui_out::do_message (const char *format, va_list args)
+mi_ui_out::do_message (ui_out_style_kind style,
+		       const char *format, va_list args)
 {
 }
 
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 82f77592da8..914a44194d4 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -64,8 +64,9 @@ protected:
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (const char *format, va_list args) override
-    ATTRIBUTE_PRINTF (2,0);
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args) override
+    ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 130d5cd48ff..5696d6fa890 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4603,9 +4603,10 @@ print_symbol_info (enum search_domain kind,
 
       if (filename_cmp (last, s_filename) != 0)
 	{
-	  fputs_filtered ("\nFile ", gdb_stdout);
-	  fputs_styled (s_filename, file_name_style.style (), gdb_stdout);
-	  fputs_filtered (":\n", gdb_stdout);
+	  current_uiout->message ("\nFile %pS%s%pN:\n",
+				  ptr (ui_out_style_kind::FILE),
+				  s_filename,
+				  nullptr);
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4651,15 +4652,16 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
   else
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
-  fputs_styled (tmp, address_style.style (), gdb_stdout);
-  fputs_filtered ("  ", gdb_stdout);
-  if (msymbol.minsym->text_p ())
-    fputs_styled (MSYMBOL_PRINT_NAME (msymbol.minsym),
-		  function_name_style.style (),
-		  gdb_stdout);
-  else
-    fputs_filtered (MSYMBOL_PRINT_NAME (msymbol.minsym), gdb_stdout);
-  fputs_filtered ("\n", gdb_stdout);
+
+  current_uiout->message (_("%pS%s%pN  %pS%s%pN\n"),
+			  ptr (ui_out_style_kind::ADDRESS),
+			  tmp,
+			  nullptr,
+			  (msymbol.minsym->text_p ()
+			   ? ptr (ui_out_style_kind::FUNCTION)
+			   : ptr (ui_out_style_kind::DEFAULT)),
+			   MSYMBOL_PRINT_NAME (msymbol.minsym),
+			  nullptr);
 }
 
 /* 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 9c987a6cfa6..6e1c87ab841 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -548,12 +548,12 @@ ui_out::text (const char *string)
 }
 
 void
-ui_out::call_do_message (const char *format, ...)
+ui_out::call_do_message (ui_out_style_kind style, const char *format, ...)
 {
   va_list args;
 
   va_start (args, format);
-  do_message (format, args);
+  do_message (style, format, args);
   va_end (args);
 }
 
@@ -562,6 +562,8 @@ ui_out::message (const char *format, ...)
 {
   format_pieces fpieces (&format, true);
 
+  ui_out_style_kind style = ui_out_style_kind::DEFAULT;
+
   va_list args;
   va_start (args, format);
 
@@ -572,7 +574,7 @@ ui_out::message (const char *format, ...)
       switch (piece.argclass)
 	{
 	case string_arg:
-	  call_do_message (current_substring, va_arg (args, const char *));
+	  call_do_message (style, current_substring, va_arg (args, const char *));
 	  break;
 	case wide_string_arg:
 	  /* FIXME */
@@ -582,16 +584,16 @@ ui_out::message (const char *format, ...)
 	  break;
 	case long_long_arg:
 #ifdef PRINTF_HAS_LONG_LONG
-	  call_do_message (current_substring, va_arg (args, long long));
+	  call_do_message (style, current_substring, va_arg (args, long long));
 	  break;
 #else
 	  error (_("long long not supported in ui_out::message"));
 #endif
 	case int_arg:
-	  call_do_message (current_substring, va_arg (args, int));
+	  call_do_message (style, current_substring, va_arg (args, int));
 	  break;
 	case long_arg:
-	  call_do_message (current_substring, va_arg (args, long));
+	  call_do_message (style, current_substring, va_arg (args, long));
 	  break;
 	  /* Handle floating-point values.  */
 	case double_arg:
@@ -611,13 +613,14 @@ ui_out::message (const char *format, ...)
 	      }
 	      break;
 	    case 'S':
-	      /* Push style on stack?  */
+	      style = *va_arg (args, ui_out_style_kind *);
 	      break;
 	    case 'N':
-	      /* Pop style from stack?  */
+	      va_arg (args, void *);
+	      style = ui_out_style_kind::DEFAULT;
 	      break;
 	    default:
-	      call_do_message (current_substring, va_arg (args, void *));
+	      call_do_message (style, current_substring, va_arg (args, void *));
 	      break;
 	    }
 	  break;
@@ -629,7 +632,7 @@ ui_out::message (const char *format, ...)
 	     pass a dummy argument because some platforms have
 	     modified GCC to include -Wformat-security by default,
 	     which will warn here if there is no argument.  */
-	  call_do_message (current_substring, 0);
+	  call_do_message (style, current_substring, 0);
 	  break;
 	default:
 	  internal_error (__FILE__, __LINE__,
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index b20d9508330..18af856313d 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -103,6 +103,24 @@ private:
   int m_val;
 };
 
+/* Wrap a ui_out_style_kind in a pointer to a temporary.  */
+
+/* XXX: Make a template?  */
+struct ptrS
+{
+  ptrS (ui_out_style_kind t) : m_t (t) {}
+
+  const ui_out_style_kind *ptr () const { return &m_t; }
+
+  ui_out_style_kind m_t;
+};
+
+static inline const ui_out_style_kind *
+ptr (ptrS &&t)
+{
+  return t.ptr ();
+}
+
 class ui_out
 {
  public:
@@ -188,8 +206,9 @@ class ui_out
     ATTRIBUTE_PRINTF (6,0) = 0;
   virtual void do_spaces (int numspaces) = 0;
   virtual void do_text (const char *string) = 0;
-  virtual void do_message (const char *format, va_list args)
-    ATTRIBUTE_PRINTF (2,0) = 0;
+  virtual void do_message (ui_out_style_kind style,
+			   const char *format, va_list args)
+    ATTRIBUTE_PRINTF (3,0) = 0;
   virtual void do_wrap_hint (const char *identstring) = 0;
   virtual void do_flush () = 0;
   virtual void do_redirect (struct ui_file *outstream) = 0;
@@ -201,7 +220,7 @@ class ui_out
   { return false; }
 
  private:
-  void call_do_message (const char *format, ...);
+  void call_do_message (ui_out_style_kind style, const char *format, ...);
 
   ui_out_flags m_flags;
 
diff --git a/gdb/utils.c b/gdb/utils.c
index f55661287eb..6ba4e41faa2 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2147,6 +2147,16 @@ fprintf_styled (struct ui_file *stream, const ui_file_style &style,
   set_output_style (stream, ui_file_style ());
 }
 
+/* See utils.h.  */
+
+void
+vfprintf_styled (struct ui_file *stream, const ui_file_style &style,
+		 const char *format, va_list args)
+{
+  set_output_style (stream, style);
+  vfprintf_filtered (stream, format, args);
+  set_output_style (stream, ui_file_style ());
+}
 
 void
 printf_filtered (const char *format, ...)
diff --git a/gdb/utils.h b/gdb/utils.h
index 58b4a283447..bb73d4a8e72 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -429,6 +429,12 @@ extern void fprintf_styled (struct ui_file *stream,
 			    ...)
   ATTRIBUTE_PRINTF (3, 4);
 
+extern void vfprintf_styled (struct ui_file *stream,
+			     const ui_file_style &style,
+			     const char *fmt,
+			     va_list args)
+  ATTRIBUTE_PRINTF (3, 0);
+
 /* Like fputs_filtered, but styles the output according to STYLE, when
    appropriate.  */
 
-- 
2.14.5

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

[Pedro Alves]

On 6/5/19 9:39 PM, Pedro Alves wrote:

> The format strings do get a bit ... wild:
> 
>    "%pS%s%pN  %pS%s%pN\n"
> 
> With your suggestion, it'd look like:
> 
>    "%p[%s%p]  %p[%s%p]\n"
> 
> I'm thinking that %pS/%pN %p[/%pN] are more appropriate when
> the styled string is constant, like:
> 
>    "this %p[text here%p] should be styled\n"
> 
> For the seemingly common case of printing a string variable
> with a style, I'm thinking that a specific formatter would
> be better.  I'll post a follow up patch for that.

Here it is.  I used %ps.

I pushed these two to the branch, in case you want to play with
them.  (I'm not likely to play with this any more today.)

From 52bf8432db926e2ce2ff8bdbececd242e93b4ca7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Jun 2019 21:15:24 +0100
Subject: [PATCH 2/2] Introduce %ps / styled_string

---
 gdb/common/format.c |  1 +
 gdb/symtab.c        | 24 +++++++++++-------------
 gdb/ui-out.c        |  6 ++++++
 gdb/ui-out.h        | 20 ++++++++++++++++++++
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/gdb/common/format.c b/gdb/common/format.c
index d33eab2b2b0..177f79afee3 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -256,6 +256,7 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 	      {
 		switch (f[1])
 		  {
+		  case 's':
 		  case 'S':
 		  case 'F':
 		  case 'N':
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 5696d6fa890..7dbf24eb7c5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4603,10 +4603,9 @@ print_symbol_info (enum search_domain kind,
 
       if (filename_cmp (last, s_filename) != 0)
 	{
-	  current_uiout->message ("\nFile %pS%s%pN:\n",
-				  ptr (ui_out_style_kind::FILE),
-				  s_filename,
-				  nullptr);
+	  current_uiout->message
+	    (_("\nFile %ps:\n"),
+	     styled_string (ui_out_style_kind::FILE, s_filename).ptr ());
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4653,15 +4652,14 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
 
-  current_uiout->message (_("%pS%s%pN  %pS%s%pN\n"),
-			  ptr (ui_out_style_kind::ADDRESS),
-			  tmp,
-			  nullptr,
-			  (msymbol.minsym->text_p ()
-			   ? ptr (ui_out_style_kind::FUNCTION)
-			   : ptr (ui_out_style_kind::DEFAULT)),
-			   MSYMBOL_PRINT_NAME (msymbol.minsym),
-			  nullptr);
+  ui_out_style_kind sym_style = (msymbol.minsym->text_p ()
+				 ? ui_out_style_kind::FUNCTION
+				 : ui_out_style_kind::DEFAULT);
+
+  current_uiout->message
+    (_("%ps  %ps\n"),
+     styled_string (ui_out_style_kind::ADDRESS, tmp).ptr (),
+     styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ());
 }
 
 /* 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 6e1c87ab841..4169fdcb935 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -612,6 +612,12 @@ ui_out::message (const char *format, ...)
 		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 ());
+	      }
+	      break;
 	    case 'S':
 	      style = *va_arg (args, ui_out_style_kind *);
 	      break;
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 18af856313d..059d1e376aa 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -103,6 +103,26 @@ private:
   int m_val;
 };
 
+struct styled_string
+{
+  styled_string (ui_out_style_kind style, const char *str)
+    : m_style (style),
+      m_str (str)
+  {
+  }
+
+  /* We need this because we can't pass a reference via
+     va_args.  */
+  const styled_string *ptr () const { return this; }
+
+  ui_out_style_kind style () const {return m_style; }
+  const char *str () const {return m_str; }
+
+private:
+  ui_out_style_kind m_style;
+  const char *m_str;
+};
+
 /* Wrap a ui_out_style_kind in a pointer to a temporary.  */
 
 /* XXX: Make a template?  */
-- 
2.14.5

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Here it is.  I used %ps.

Looks good to me.

Tom

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Also, since gcc expects a pointer for %p, and we want to pass an
Pedro> enum for the style, I added a small pointer-wrapper hack -- see the
Pedro> ptr function.  Yay C++.

We could just remove the enum, move to passing pointers everywhere, and
convert the *_style objects to be pointers.  This would streamline
things a bit; and we could use nullptr to mean "keep the default".

Another option would be to just add a ptr method to the _style objects,
so instead of ui_out_style_kind::VARIABLE you'd write variable_style.ptr ().

Pedro> I've not been paying much attention to the styling patches, so I can't
Pedro> off hand tell which places would benefit the most from this.  So I just
Pedro> grepped for _styled and replaced a couple spots.  Likely there are better
Pedro> examples.

There's my new patch and I have one more along those lines as well, but
a good existing one that was already deconstructed is in symfile.c:

	  puts_filtered (_("Reading symbols from "));
	  fputs_styled (name, file_name_style.style (), gdb_stdout);
	  puts_filtered ("...\n");

Pedro> For the seemingly common case of printing a string variable
Pedro> with a style, I'm thinking that a specific formatter would
Pedro> be better.  I'll post a follow up patch for that.

I'm interested to see it.

Tom

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

[Pedro Alves]

On 6/5/19 9:47 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Also, since gcc expects a pointer for %p, and we want to pass an
> Pedro> enum for the style, I added a small pointer-wrapper hack -- see the
> Pedro> ptr function.  Yay C++.
> 
> We could just remove the enum, move to passing pointers everywhere, and
> convert the *_style objects to be pointers.  This would streamline
> things a bit; 

...

> and we could use nullptr to mean "keep the default".

Yeah, that would %pN / %p] redundant/unnecessary...

> 
> Another option would be to just add a ptr method to the _style objects,
> so instead of ui_out_style_kind::VARIABLE you'd write variable_style.ptr ().

... and could thus end up with something like:

  current_uiout->message (_("%pS%s%pS  %pS%s%pS\n"),
			  address_style.style ().ptr (),
			  tmp,
			  nullptr,
			  (msymbol.minsym->text_p ()
			   ? function_name_style.style ().ptr ()
			   : nullptr),
			   MSYMBOL_PRINT_NAME (msymbol.minsym),
			  nullptr);

If you'd like to give this a try, please do feel free to push to
the branch.

> 
> Pedro> I've not been paying much attention to the styling patches, so I can't
> Pedro> off hand tell which places would benefit the most from this.  So I just
> Pedro> grepped for _styled and replaced a couple spots.  Likely there are better
> Pedro> examples.
> 
> There's my new patch and I have one more along those lines as well, but
> a good existing one that was already deconstructed is in symfile.c:
> 
> 	  puts_filtered (_("Reading symbols from "));
> 	  fputs_styled (name, file_name_style.style (), gdb_stdout);
> 	  puts_filtered ("...\n");

So that could be either:

current_uiout->message (_("Reading symbols from %pS%s%pS...\n"),
                        file_name_style.style ().ptr (),
                        name,
                        nullptr);

or:

current_uiout->message (_("Reading symbols from %ps...\n"),
                        styled_string (ui_out_style_kind::FILENAME, name).ptr ());

or even:

current_uiout->message (_("Reading symbols from %ps...\n"),
                        styled_string (file_name_style.style (), name).ptr ());

> 
> Pedro> For the seemingly common case of printing a string variable
> Pedro> with a style, I'm thinking that a specific formatter would
> Pedro> be better.  I'll post a follow up patch for that.
> 
> I'm interested to see it.
> 
Thanks,
Pedro Alves

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

[Tom Tromey]

>> We could just remove the enum, move to passing pointers everywhere, and
>> convert the *_style objects to be pointers.  This would streamline
>> things a bit; 

Pedro> ...

>> and we could use nullptr to mean "keep the default".

Pedro> Yeah, that would %pN / %p] redundant/unnecessary...

I didn't go quite as far as making everything take pointers, partly
because it made it harder to preserve the distinction between the CLI
layer and the ui-file styling layer.  So instead I just removed the
enum.

My patch also removes (really just comments out) %pS and %pN in favor of
requiring %ps.  Those could be restored with a bit of effort I guess.

Pedro> If you'd like to give this a try, please do feel free to push to
Pedro> the branch.

I didn't push but my patch is appended.  Let me know what you think.  I
can push tomorrow if you want it.  I'm not sure now if what remains of
the patch is a good idea or not; or if moving fully to pointers would be
better.

Tom

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0c3c7df92fb..c5d3ccab1eb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5836,14 +5836,14 @@ print_breakpoint_location (struct breakpoint *b,
 	{
 	  uiout->text ("in ");
 	  uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	  uiout->text (" ");
 	  uiout->wrap_hint (wrap_indent_at_field (uiout, "what"));
 	  uiout->text ("at ");
 	}
       uiout->field_string ("file",
 			   symtab_to_filename_for_display (loc->symtab),
-			   ui_out_style_kind::FILE);
+			   file_name_style.style ());
       uiout->text (":");
 
       if (uiout->is_mi_like_p ())
@@ -6693,9 +6693,9 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
 			     (others > 1) ? "," 
 			     : ((others == 1) ? " and" : ""));
 	  }
-      current_uiout->message (_("also set at pc %pS%s%pN.\n"),
-			      ptr (ui_out_style_kind::ADDRESS),
-			      paddress (gdbarch, pc), nullptr);
+      current_uiout->message (_("also set at pc %ps.\n"),
+			      styled_string (address_style.style (),
+					     paddress (gdbarch, pc)).ptr ());
     }
 }
 \f
@@ -13355,12 +13355,12 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal)
 	  if (sym)
 	    {
 	      uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-				   ui_out_style_kind::FUNCTION);
+				   function_name_style.style ());
 	      uiout->text (" at ");
 	    }
 	  uiout->field_string ("file",
 			       symtab_to_filename_for_display (sal2.symtab),
-			       ui_out_style_kind::FILE);
+			       file_name_style.style ());
 	  uiout->text (":");
 
 	  if (uiout->is_mi_like_p ())
diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 71ed5c3cf33..aaea20e1512 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -73,7 +73,7 @@ cli_ui_out::do_table_header (int width, ui_align alignment,
     return;
 
   do_field_string (0, width, alignment, 0, col_hdr.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* Mark beginning of a list */
@@ -102,7 +102,7 @@ cli_ui_out::do_field_int (int fldno, int width, ui_align alignment,
   std::string str = string_printf ("%d", value);
 
   do_field_string (fldno, width, alignment, fldname, str.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* used to omit a field */
@@ -115,28 +115,7 @@ cli_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
     return;
 
   do_field_string (fldno, width, alignment, fldname, "",
-		   ui_out_style_kind::DEFAULT);
-}
-
-static ui_file_style
-style_from_style_kind (ui_out_style_kind kind)
-{
-  switch (kind)
-    {
-    case ui_out_style_kind::DEFAULT:
-      /* Nothing.  */
-      return {};
-    case ui_out_style_kind::FILE:
-      return file_name_style.style ();
-    case ui_out_style_kind::FUNCTION:
-      return function_name_style.style ();
-    case ui_out_style_kind::VARIABLE:
-      return variable_name_style.style ();
-    case ui_out_style_kind::ADDRESS:
-      return address_style.style ();
-    default:
-      gdb_assert_not_reached ("missing case");
-    }
+		   ui_file_style ());
 }
 
 /* other specific cli_field_* end up here so alignment and field
@@ -145,7 +124,7 @@ style_from_style_kind (ui_out_style_kind kind)
 void
 cli_ui_out::do_field_string (int fldno, int width, ui_align align,
 			     const char *fldname, const char *string,
-			     ui_out_style_kind style)
+			     const ui_file_style &style)
 {
   int before = 0;
   int after = 0;
@@ -180,10 +159,7 @@ cli_ui_out::do_field_string (int fldno, int width, ui_align align,
     spaces (before);
 
   if (string)
-    {
-      ui_file_style fstyle = style_from_style_kind (style);
-      fputs_styled (string, fstyle, m_streams.back ());
-    }
+    fputs_styled (string, style, m_streams.back ());
 
   if (after)
     spaces (after);
@@ -205,7 +181,7 @@ cli_ui_out::do_field_fmt (int fldno, int width, ui_align align,
   std::string str = string_vprintf (format, args);
 
   do_field_string (fldno, width, align, fldname, str.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 void
@@ -227,14 +203,13 @@ cli_ui_out::do_text (const char *string)
 }
 
 void
-cli_ui_out::do_message (ui_out_style_kind style,
+cli_ui_out::do_message (const ui_file_style &style,
 			const char *format, va_list args)
 {
   if (m_suppress_output)
     return;
 
-  ui_file_style fstyle = style_from_style_kind (style);
-  vfprintf_styled (m_streams.back (), fstyle, format, args);
+  vfprintf_styled (m_streams.back (), style, format, args);
 }
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 4ed7b396a57..eb519bb4a8d 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -52,14 +52,14 @@ protected:
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname,
 				const char *string,
-				ui_out_style_kind style) override;
+				const ui_file_style &style) override;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			     const char *fldname, const char *format,
 			     va_list args)
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args) override
     ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
diff --git a/gdb/disasm.c b/gdb/disasm.c
index ed740c26e0f..54401a975b9 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -31,6 +31,7 @@
 #include <algorithm>
 #include "common/gdb_optional.h"
 #include "valprint.h"
+#include "cli/cli-style.h"
 
 /* Disassemble functions.
    FIXME: We should get rid of all the duplicate code in gdb that does
@@ -245,7 +246,7 @@ gdb_pretty_print_disassembler::pretty_print_insn (struct ui_out *uiout,
 	uiout->text (" <");
 	if ((flags & DISASSEMBLY_OMIT_FNAME) == 0)
 	  uiout->field_string ("func-name", name.c_str (),
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	uiout->text ("+");
 	uiout->field_int ("offset", offset);
 	uiout->text (">:\t");
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index bb52360e540..b8ee8006018 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -70,9 +70,9 @@ mi_ui_out::do_table_header (int width, ui_align alignment,
   do_field_int (0, 0, ui_center, "width", width);
   do_field_int (0, 0, ui_center, "alignment", alignment);
   do_field_string (0, 0, ui_center, "col_name", col_name.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
   do_field_string (0, width, alignment, "colhdr", col_hdr.c_str (),
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
   close (ui_out_type_tuple);
 }
 
@@ -102,7 +102,7 @@ mi_ui_out::do_field_int (int fldno, int width, ui_align alignment,
 
   xsnprintf (buffer, sizeof (buffer), "%d", value);
   do_field_string (fldno, width, alignment, fldname, buffer,
-		   ui_out_style_kind::DEFAULT);
+		   ui_file_style ());
 }
 
 /* Used to omit a field.  */
@@ -119,7 +119,7 @@ mi_ui_out::do_field_skip (int fldno, int width, ui_align alignment,
 void
 mi_ui_out::do_field_string (int fldno, int width, ui_align align,
 			    const char *fldname, const char *string,
-			    ui_out_style_kind style)
+			    const ui_file_style &style)
 {
   ui_file *stream = m_streams.back ();
   field_separator ();
@@ -159,7 +159,7 @@ mi_ui_out::do_text (const char *string)
 }
 
 void
-mi_ui_out::do_message (ui_out_style_kind style,
+mi_ui_out::do_message (const ui_file_style &style,
 		       const char *format, va_list args)
 {
 }
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 914a44194d4..f01d62f9bf3 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -58,13 +58,13 @@ protected:
 			   const char *fldname) override;
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname, const char *string,
-				ui_out_style_kind style) override;
+				const ui_file_style &style) override;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			  const char *fldname, const char *format, va_list args)
     override ATTRIBUTE_PRINTF (6,0);
   virtual void do_spaces (int numspaces) override;
   virtual void do_text (const char *string) override;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args) override
     ATTRIBUTE_PRINTF (3,0);
   virtual void do_wrap_hint (const char *identstring) override;
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 95ad410f23f..0d084564c20 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -31,6 +31,7 @@
 #include "mi/mi-cmds.h"
 #include "python-internal.h"
 #include "common/gdb_optional.h"
+#include "cli/cli-style.h"
 
 enum mi_print_types
 {
@@ -898,7 +899,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 	  if (function == NULL)
 	    out->field_skip ("func");
 	  else
-	    out->field_string ("func", function, ui_out_style_kind::FUNCTION);
+	    out->field_string ("func", function, function_name_style.style ());
 	}
     }
 
@@ -935,7 +936,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 	      out->text (" at ");
 	      annotate_frame_source_file ();
 	      out->field_string ("file", filename.get (),
-				 ui_out_style_kind::FILE);
+				 file_name_style.style ());
 	      annotate_frame_source_file_end ();
 	    }
 	}
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 21085d5c62c..02bc43810f0 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -41,6 +41,7 @@
 #include "common/vec.h"
 #include "inferior.h"
 #include <algorithm>
+#include "cli/cli-style.h"
 
 static const target_info record_btrace_target_info = {
   "record-btrace",
@@ -1091,7 +1092,7 @@ btrace_call_history_src_line (struct ui_out *uiout,
 
   uiout->field_string ("file",
 		       symtab_to_filename_for_display (symbol_symtab (sym)),
-		       ui_out_style_kind::FILE);
+		       file_name_style.style ());
 
   btrace_compute_src_line_range (bfun, &begin, &end);
   if (end < begin)
@@ -1183,13 +1184,13 @@ btrace_call_history (struct ui_out *uiout,
 
       if (sym != NULL)
 	uiout->field_string ("function", SYMBOL_PRINT_NAME (sym),
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
       else if (msym != NULL)
 	uiout->field_string ("function", MSYMBOL_PRINT_NAME (msym),
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
       else if (!uiout->is_mi_like_p ())
 	uiout->field_string ("function", "??",
-			     ui_out_style_kind::FUNCTION);
+			     function_name_style.style ());
 
       if ((flags & RECORD_PRINT_INSN_RANGE) != 0)
 	{
diff --git a/gdb/skip.c b/gdb/skip.c
index 127b11dc443..0578422b99c 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -36,6 +36,7 @@
 #include "gdb_regex.h"
 #include "common/gdb_optional.h"
 #include <list>
+#include "cli/cli-style.h"
 
 /* True if we want to print debug printouts related to file/function
    skipping. */
@@ -414,7 +415,7 @@ info_skip_command (const char *arg, int from_tty)
       current_uiout->field_string ("file",
 				   e.file ().empty () ? "<none>"
 				   : e.file ().c_str (),
-				   ui_out_style_kind::FILE); /* 4 */
+				   file_name_style.style ()); /* 4 */
       if (e.function_is_regexp ())
 	current_uiout->field_string ("regexp", "y"); /* 5 */
       else
@@ -423,7 +424,7 @@ info_skip_command (const char *arg, int from_tty)
       current_uiout->field_string ("function",
 				   e.function ().empty () ? "<none>"
 				   : e.function ().c_str (),
-				   ui_out_style_kind::FUNCTION); /* 6 */
+				   function_name_style.style ()); /* 6 */
 
       current_uiout->text ("\n");
     }
diff --git a/gdb/solib.c b/gdb/solib.c
index e0b1a921f82..662433d33d1 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -47,6 +47,7 @@
 #include "gdb_bfd.h"
 #include "common/filestuff.h"
 #include "source.h"
+#include "cli/cli-style.h"
 
 /* Architecture-specific operations.  */
 
@@ -1104,7 +1105,7 @@ info_sharedlibrary_command (const char *pattern, int from_tty)
 	else
 	  uiout->field_string ("syms-read", so->symbols_loaded ? "Yes" : "No");
 
-	uiout->field_string ("name", so->so_name, ui_out_style_kind::FILE);
+	uiout->field_string ("name", so->so_name, file_name_style.style ());
 
 	uiout->text ("\n");
       }
diff --git a/gdb/source.c b/gdb/source.c
index 9a30209880b..3f6f64e1fdd 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -46,6 +46,7 @@
 #include <algorithm>
 #include "common/pathstuff.h"
 #include "source-cache.h"
+#include "cli/cli-style.h"
 
 #define OPEN_MODE (O_RDONLY | O_BINARY)
 #define FDOPEN_MODE FOPEN_RB
@@ -1306,7 +1307,7 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
 	     not for TUI.  */
 	  if (uiout->is_mi_like_p () || uiout->test_flags (ui_source_list))
 	    uiout->field_string ("file", symtab_to_filename_for_display (s),
-				 ui_out_style_kind::FILE);
+				 file_name_style.style ());
 	  if (uiout->is_mi_like_p () || !uiout->test_flags (ui_source_list))
  	    {
 	      const char *s_fullname = symtab_to_fullname (s);
diff --git a/gdb/stack.c b/gdb/stack.c
index 547e82bbfb2..d8c6fdf550b 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -53,6 +53,7 @@
 #include "observable.h"
 #include "common/def-vector.h"
 #include "cli/cli-option.h"
+#include "cli/cli-style.h"
 
 /* The possible choices of "set print frame-arguments", and the value
    of this setting.  */
@@ -341,7 +342,7 @@ print_frame_arg (const frame_print_options &fp_opts,
   if (arg->entry_kind == print_entry_values_only
       || arg->entry_kind == print_entry_values_compact)
     stb.puts ("@entry");
-  uiout->field_stream ("name", stb, ui_out_style_kind::VARIABLE);
+  uiout->field_stream ("name", stb, variable_name_style.style ());
   annotate_arg_name_end ();
   uiout->text ("=");
 
@@ -909,18 +910,18 @@ print_frame_info (const frame_print_options &fp_opts,
         {
           annotate_function_call ();
           uiout->field_string ("func", "<function called from gdb>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	}
       else if (get_frame_type (frame) == SIGTRAMP_FRAME)
         {
 	  annotate_signal_handler_caller ();
           uiout->field_string ("func", "<signal handler called>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
         }
       else if (get_frame_type (frame) == ARCH_FRAME)
         {
           uiout->field_string ("func", "<cross-architecture call>",
-			       ui_out_style_kind::FUNCTION);
+			       function_name_style.style ());
 	}
       uiout->text ("\n");
       annotate_frame_end ();
@@ -1262,7 +1263,7 @@ print_frame (const frame_print_options &fp_opts,
 	    uiout->field_core_addr ("addr", gdbarch, pc);
 	  else
 	    uiout->field_string ("addr", "<unavailable>",
-				 ui_out_style_kind::ADDRESS);
+				 address_style.style ());
 	  annotate_frame_address_end ();
 	  uiout->text (" in ");
 	}
@@ -1271,7 +1272,7 @@ print_frame (const frame_print_options &fp_opts,
     string_file stb;
     fprintf_symbol_filtered (&stb, funname ? funname.get () : "??",
 			     funlang, DMGL_ANSI);
-    uiout->field_stream ("func", stb, ui_out_style_kind::FUNCTION);
+    uiout->field_stream ("func", stb, function_name_style.style ());
     uiout->wrap_hint ("   ");
     annotate_frame_args ();
 
@@ -1313,7 +1314,8 @@ print_frame (const frame_print_options &fp_opts,
 	uiout->wrap_hint ("   ");
 	uiout->text (" at ");
 	annotate_frame_source_file ();
-	uiout->field_string ("file", filename_display, ui_out_style_kind::FILE);
+	uiout->field_string ("file", filename_display,
+			     file_name_style.style ());
 	if (uiout->is_mi_like_p ())
 	  {
 	    const char *fullname = symtab_to_fullname (sal.symtab);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 26762d289ca..903f2ff817b 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -1115,11 +1115,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
       if (deprecated_pre_add_symbol_hook)
 	deprecated_pre_add_symbol_hook (name);
       else
-	{
-	  puts_filtered (_("Reading symbols from "));
-	  fputs_styled (name, file_name_style.style (), gdb_stdout);
-	  puts_filtered ("...\n");
-	}
+	current_uiout->message (_("Reading symbols from %ps...\n"),
+				styled_string (file_name_style.style (),
+					       name).ptr ());
     }
   syms_from_objfile (objfile, addrs, add_flags);
 
@@ -1131,7 +1129,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
   if ((flags & OBJF_READNOW))
     {
       if (should_print)
-	printf_filtered (_("Expanding full symbols from %s...\n"), name);
+	current_uiout->message (_("Expanding full symbols from %ps...\n"),
+				styled_string (file_name_style.style (),
+					       name).ptr ());
 
       if (objfile->sf)
 	objfile->sf->qf->expand_all_symtabs (objfile);
@@ -1143,7 +1143,9 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
      file, and so printing it twice is just redundant.  */
   if (should_print && !objfile_has_symbols (objfile)
       && objfile->separate_debug_objfile == nullptr)
-    printf_filtered (_("(No debugging symbols found in %s)\n"), name);
+    current_uiout->message (_("(No debugging symbols found in %ps)\n"),
+			    styled_string (file_name_style.style (),
+					   name).ptr ());
 
   if (should_print)
     {
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 7dbf24eb7c5..e5f702ab804 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 (ui_out_style_kind::FILE, s_filename).ptr ());
+	     styled_string (file_name_style.style (), s_filename).ptr ());
 	}
 
       if (SYMBOL_LINE (sym) != 0)
@@ -4652,13 +4652,13 @@ print_msymbol_info (struct bound_minimal_symbol msymbol)
     tmp = hex_string_custom (BMSYMBOL_VALUE_ADDRESS (msymbol),
 			     16);
 
-  ui_out_style_kind sym_style = (msymbol.minsym->text_p ()
-				 ? ui_out_style_kind::FUNCTION
-				 : ui_out_style_kind::DEFAULT);
+  ui_file_style sym_style = (msymbol.minsym->text_p ()
+			     ? function_name_style.style ()
+			     : ui_file_style ());
 
   current_uiout->message
     (_("%ps  %ps\n"),
-     styled_string (ui_out_style_kind::ADDRESS, tmp).ptr (),
+     styled_string (address_style.style (), tmp).ptr (),
      styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ());
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index c7585c67839..170c40a3046 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -57,6 +57,7 @@
 #include "tracefile.h"
 #include "location.h"
 #include <algorithm>
+#include "cli/cli-style.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -3690,7 +3691,7 @@ print_one_static_tracepoint_marker (int count,
     {
       uiout->text ("in ");
       uiout->field_string ("func", SYMBOL_PRINT_NAME (sym),
-			   ui_out_style_kind::FUNCTION);
+			   function_name_style.style ());
       uiout->wrap_hint (wrap_indent);
       uiout->text (" at ");
     }
@@ -3701,7 +3702,7 @@ print_one_static_tracepoint_marker (int count,
     {
       uiout->field_string ("file",
 			   symtab_to_filename_for_display (sal.symtab),
-			   ui_out_style_kind::FILE);
+			   file_name_style.style ());
       uiout->text (":");
 
       if (uiout->is_mi_like_p ())
diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index 5fabff2cf1c..521c519cb75 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -52,7 +52,7 @@ tui_ui_out::do_field_int (int fldno, int width, ui_align alignment,
 void
 tui_ui_out::do_field_string (int fldno, int width, ui_align align,
 			     const char *fldname, const char *string,
-			     ui_out_style_kind style)
+			     const ui_file_style &style)
 {
   if (suppress_output ())
     return;
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index edf0b912bef..173027df525 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -35,7 +35,7 @@ protected:
   void do_field_int (int fldno, int width, ui_align align, const char *fldname,
 		  int value) override;
   void do_field_string (int fldno, int width, ui_align align, const char *fldname,
-			const char *string, ui_out_style_kind style) override;
+			const char *string, const ui_file_style &style) override;
   void do_field_fmt (int fldno, int width, ui_align align, const char *fldname,
 		  const char *format, va_list args) override
     ATTRIBUTE_PRINTF (6,0);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 4169fdcb935..e74f685af02 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -25,6 +25,7 @@
 #include "language.h"
 #include "ui-out.h"
 #include "common/format.h"
+#include "cli/cli-style.h"
 
 #include <vector>
 #include <memory>
@@ -470,12 +471,12 @@ ui_out::field_core_addr (const char *fldname, struct gdbarch *gdbarch,
 			 CORE_ADDR address)
 {
   field_string (fldname, print_core_address (gdbarch, address),
-		ui_out_style_kind::ADDRESS);
+		address_style.style ());
 }
 
 void
 ui_out::field_stream (const char *fldname, string_file &stream,
-		      ui_out_style_kind style)
+		      const ui_file_style &style)
 {
   if (!stream.empty ())
     field_string (fldname, stream.c_str (), style);
@@ -500,7 +501,7 @@ ui_out::field_skip (const char *fldname)
 
 void
 ui_out::field_string (const char *fldname, const char *string,
-		      ui_out_style_kind style)
+		      const ui_file_style &style)
 {
   int fldno;
   int width;
@@ -548,7 +549,8 @@ ui_out::text (const char *string)
 }
 
 void
-ui_out::call_do_message (ui_out_style_kind style, const char *format, ...)
+ui_out::call_do_message (const ui_file_style &style, const char *format,
+			 ...)
 {
   va_list args;
 
@@ -562,7 +564,7 @@ ui_out::message (const char *format, ...)
 {
   format_pieces fpieces (&format, true);
 
-  ui_out_style_kind style = ui_out_style_kind::DEFAULT;
+  ui_file_style style;
 
   va_list args;
   va_start (args, format);
@@ -618,13 +620,13 @@ ui_out::message (const char *format, ...)
 		call_do_message (ss->style (), "%s", ss->str ());
 	      }
 	      break;
-	    case 'S':
-	      style = *va_arg (args, ui_out_style_kind *);
-	      break;
-	    case 'N':
-	      va_arg (args, void *);
-	      style = ui_out_style_kind::DEFAULT;
-	      break;
+	    /* case 'S': */
+	    /*   style = *va_arg (args, const ui_file_style *); */
+	    /*   break; */
+	    /* case 'N': */
+	    /*   va_arg (args, void *); */
+	    /*   style = nullptr; */
+	    /*   break; */
 	    default:
 	      call_do_message (style, current_substring, va_arg (args, void *));
 	      break;
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 059d1e376aa..f4c3857fe2d 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -26,6 +26,7 @@
 #include <vector>
 
 #include "common/enum-flags.h"
+#include "ui-style.h"
 
 class ui_out_level;
 class ui_out_table;
@@ -67,22 +68,6 @@ enum ui_out_type
     ui_out_type_list
   };
 
-/* Possible kinds of styling.  */
-
-enum class ui_out_style_kind
-{
-  /* The default (plain) style.  */
-  DEFAULT,
-  /* File name.  */
-  FILE,
-  /* Function name.  */
-  FUNCTION,
-  /* Variable name.  */
-  VARIABLE,
-  /* Address.  */
-  ADDRESS
-};
-
 struct int_field
 {
   int_field (const char *name, int val)
@@ -105,7 +90,7 @@ private:
 
 struct styled_string
 {
-  styled_string (ui_out_style_kind style, const char *str)
+  styled_string (const ui_file_style &style, const char *str)
     : m_style (style),
       m_str (str)
   {
@@ -115,32 +100,14 @@ struct styled_string
      va_args.  */
   const styled_string *ptr () const { return this; }
 
-  ui_out_style_kind style () const {return m_style; }
+  const ui_file_style &style () const { return m_style; }
   const char *str () const {return m_str; }
 
 private:
-  ui_out_style_kind m_style;
+  ui_file_style m_style;
   const char *m_str;
 };
 
-/* Wrap a ui_out_style_kind in a pointer to a temporary.  */
-
-/* XXX: Make a template?  */
-struct ptrS
-{
-  ptrS (ui_out_style_kind t) : m_t (t) {}
-
-  const ui_out_style_kind *ptr () const { return &m_t; }
-
-  ui_out_style_kind m_t;
-};
-
-static inline const ui_out_style_kind *
-ptr (ptrS &&t)
-{
-  return t.ptr ();
-}
-
 class ui_out
 {
  public:
@@ -171,10 +138,10 @@ class ui_out
   void field_core_addr (const char *fldname, struct gdbarch *gdbarch,
 			CORE_ADDR address);
   void field_string (const char *fldname, const char *string,
-		     ui_out_style_kind style = ui_out_style_kind::DEFAULT);
+		     const ui_file_style &style = ui_file_style ());
   void field_string (const char *fldname, const std::string &string);
   void field_stream (const char *fldname, string_file &stream,
-		     ui_out_style_kind style = ui_out_style_kind::DEFAULT);
+		     const ui_file_style &style = ui_file_style ());
   void field_skip (const char *fldname);
   void field_fmt (const char *fldname, const char *format, ...)
     ATTRIBUTE_PRINTF (3, 4);
@@ -219,14 +186,14 @@ class ui_out
 			      const char *fldname) = 0;
   virtual void do_field_string (int fldno, int width, ui_align align,
 				const char *fldname, const char *string,
-				ui_out_style_kind style) = 0;
+				const ui_file_style &style) = 0;
   virtual void do_field_fmt (int fldno, int width, ui_align align,
 			     const char *fldname, const char *format,
 			     va_list args)
     ATTRIBUTE_PRINTF (6,0) = 0;
   virtual void do_spaces (int numspaces) = 0;
   virtual void do_text (const char *string) = 0;
-  virtual void do_message (ui_out_style_kind style,
+  virtual void do_message (const ui_file_style &style,
 			   const char *format, va_list args)
     ATTRIBUTE_PRINTF (3,0) = 0;
   virtual void do_wrap_hint (const char *identstring) = 0;
@@ -240,7 +207,8 @@ class ui_out
   { return false; }
 
  private:
-  void call_do_message (ui_out_style_kind style, const char *format, ...);
+  void call_do_message (const ui_file_style &style, const char *format,
+			...);
 
   ui_out_flags m_flags;
 

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

[Pedro Alves]

On 6/5/19 11:21 PM, Tom Tromey wrote:
>>> We could just remove the enum, move to passing pointers everywhere, and
>>> convert the *_style objects to be pointers.  This would streamline
>>> things a bit; 
> 
> Pedro> ...
> 
>>> and we could use nullptr to mean "keep the default".
> 
> Pedro> Yeah, that would %pN / %p] redundant/unnecessary...
> 
> I didn't go quite as far as making everything take pointers, partly
> because it made it harder to preserve the distinction between the CLI
> layer and the ui-file styling layer.  So instead I just removed the
> enum.
> 
> My patch also removes (really just comments out) %pS and %pN in favor of
> requiring %ps.  Those could be restored with a bit of effort I guess.

Yes, I think so.  I think the main issue is that cli_style_option::style()
returns a new temporary ui_file_style, so we can't take its address, like in:

       current_uiout->message (_("Reading symbols from %pS%s%pS...\n"),
                               &file_name_style.style (), name, nullptr);

But that could be fixed by adding a m_style field to cli_style_option,
and making cli_style_option::style() return a const reference to that.
We'd either need to make cli_style_option register set command hooks
to update that m_size variable whenever the users changes the corresponding
foreground/background setting, or always update m_style when
cli_style_option::style() is called.

> 
> Pedro> If you'd like to give this a try, please do feel free to push to
> Pedro> the branch.
> 
> I didn't push but my patch is appended.  Let me know what you think.  I
> can push tomorrow if you want it.  I'm not sure now if what remains of
> the patch is a good idea or not; or if moving fully to pointers would be
> better.
> 

Please push.  I don't know either, but undoing a patch on a branch is
simple enough if we find that we want to do that.  :-)

Thanks,
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Yes, I think so.  I think the main issue is that cli_style_option::style()
Pedro> returns a new temporary ui_file_style, so we can't take its address, like in:

If it had a 'ptr ()' method, wouldn't the temporary live long enough in
the callee for it to work?

Pedro> Please push.  I don't know either, but undoing a patch on a branch is
Pedro> simple enough if we find that we want to do that.  :-)

I did it.

Tom

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

[Tom Tromey]

Tom> My patch also removes (really just comments out) %pS and %pN in favor of
Tom> requiring %ps.  Those could be restored with a bit of effort I guess.

I think we do need these.

I have been trying a patch to style gdb's meta-data output, stuff like
<unavailable>.  There are cases where the <...> contains some other
value, like:

	  fprintf_filtered (stream, " <repeats %u times>", reps);

Here we want to rewrite as:

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

Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> My patch also removes (really just comments out) %pS and %pN in favor of
Tom> requiring %ps.  Those could be restored with a bit of effort I guess.

Tom> I think we do need these.

I pushed a couple more patches to the branch, including one intended to
fix this.

What's left to do on that branch in order to merge it?

Tom

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

[Pedro Alves]

On 6/7/19 8:20 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> My patch also removes (really just comments out) %pS and %pN in favor of
> Tom> requiring %ps.  Those could be restored with a bit of effort I guess.
> 
> Tom> I think we do need these.
> 
> I pushed a couple more patches to the branch, including one intended to
> fix this.
> 
> 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.  :-)

 @@ -12112,23 +12112,21 @@ say_where (struct breakpoint *b)
    else
      {
        if (opts.addressprint || b->loc->symtab == NULL)
 -       {
 -         printf_filtered (" at ");
 -         fputs_styled (paddress (b->loc->gdbarch, b->loc->address),
 -                       address_style.style (),
 -                       gdb_stdout);
 -       }
 +       printf_filtered (" at %ps",
 +                        styled_string (address_style.style (),
 +                                       paddress (b->loc->gdbarch,
 +                                                 b->loc->address)).ptr ());

branch, buggy:

 (top-gdb) start
 Temporary breakpoint 3 at 0x7fffc8e67590s: file 0x7fffc8e675e0s, line 28.

fixed:

 (top-gdb) start
 Temporary breakpoint 3 at 0x469a06: file /home/pedro/gdb/mygit/src/gdb/gdb.c, line 28.


If we want the new new formatters to work with printf_(un)filtered,
the patch below fixes it by making printf_(un)filtered defer to
cli_ui_out::message for format processing, instead of to
string_printf (and thus to vsnprintf).  It's maybe a bit roundabout, but
I couldn't think of a better/easier way.

From de45ea7e7544f1cb3b7facc2fa47609006d1aeb0 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 28 Jun 2019 22:40:50 +0100
Subject: [PATCH] printf_filtered

---
 gdb/cli-out.c |  5 ++++-
 gdb/ui-out.c  | 14 ++++++++++----
 gdb/ui-out.h  |  1 +
 gdb/utils.c   | 56 +++++++++++++++++++++++++++++++++++++++++++++++---------
 gdb/utils.h   |  7 +++++++
 5 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index aaea20e1512..1f95e4b8ddd 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -209,7 +209,10 @@ cli_ui_out::do_message (const ui_file_style &style,
   if (m_suppress_output)
     return;
 
-  vfprintf_styled (m_streams.back (), style, format, args);
+  /* Use the "no_gdbfmt" variant here to avoid recursion.
+     vfprintf_styled calls into cli_ui_out::message to handle the
+     gdb-specific printf formats.  */
+  vfprintf_styled_no_gdbfmt (m_streams.back (), style, format, args);
 }
 
 void
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 4116c47ea43..d50fb3a3e3f 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -560,15 +560,12 @@ ui_out::call_do_message (const ui_file_style &style, const char *format,
 }
 
 void
-ui_out::message (const char *format, ...)
+ui_out::vmessage (const char *format, va_list args)
 {
   format_pieces fpieces (&format, true);
 
   ui_file_style style;
 
-  va_list args;
-  va_start (args, format);
-
   for (auto &&piece : fpieces)
     {
       const char *current_substring = piece.string;
@@ -647,6 +644,15 @@ ui_out::message (const char *format, ...)
 			  _("failed internal consistency check"));
 	}
     }
+}
+
+void
+ui_out::message (const char *format, ...)
+{
+  va_list args;
+  va_start (args, format);
+
+  vmessage (format, args);
 
   va_end (args);
 }
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index f4c3857fe2d..35bc8b21824 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -149,6 +149,7 @@ class ui_out
   void spaces (int numspaces);
   void text (const char *string);
   void message (const char *format, ...) ATTRIBUTE_PRINTF (2, 3);
+  void vmessage (const char *format, va_list args) ATTRIBUTE_PRINTF (2, 0);
   void wrap_hint (const char *identstring);
 
   void flush ();
diff --git a/gdb/utils.c b/gdb/utils.c
index 6ba4e41faa2..22badda7713 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -73,13 +73,15 @@
 #include "common/pathstuff.h"
 #include "cli/cli-style.h"
 #include "common/scope-exit.h"
+#include "cli-out.h"
 
 void (*deprecated_error_begin_hook) (void);
 
 /* Prototypes for local functions */
 
 static void vfprintf_maybe_filtered (struct ui_file *, const char *,
-				     va_list, int) ATTRIBUTE_PRINTF (2, 0);
+				     va_list, bool, bool)
+  ATTRIBUTE_PRINTF (2, 0);
 
 static void fputs_maybe_filtered (const char *, struct ui_file *, int);
 
@@ -2027,6 +2029,26 @@ puts_debug (char *prefix, char *string, char *suffix)
     }
 }
 
+/* Like string_vprintf, but if GDBFMT is true, also process the
+   gdb-specific format specifiers.  */
+
+static std::string string_vprintf_maybe_gdbfmt (const char *format,
+						va_list args,
+						bool gdbfmt)
+  ATTRIBUTE_PRINTF (1, 0);
+
+static std::string
+string_vprintf_maybe_gdbfmt (const char *format, va_list args, bool gdbfmt)
+{
+  if (gdbfmt)
+    {
+      string_file sfile;
+      cli_ui_out (&sfile, 0).vmessage (format, args);
+      return std::move (sfile.string ());
+    }
+  else
+    return string_vprintf (format, args);
+}
 
 /* Print a variable number of ARGS using format FORMAT.  If this
    information is going to put the amount written (since the last call
@@ -2038,15 +2060,14 @@ puts_debug (char *prefix, char *string, char *suffix)
    We implement three variants, vfprintf (takes a vararg list and stream),
    fprintf (takes a stream to write on), and printf (the usual).
 
-   Note also that a longjmp to top level may occur in this routine
-   (since prompt_for_continue may do so) so this routine should not be
-   called when cleanups are not in place.  */
+   Note also that this may throw a quit (since prompt_for_continue may
+   do so).  */
 
 static void
 vfprintf_maybe_filtered (struct ui_file *stream, const char *format,
-			 va_list args, int filter)
+			 va_list args, bool filter, bool gdbfmt)
 {
-  std::string linebuffer = string_vprintf (format, args);
+  std::string linebuffer = string_vprintf_maybe_gdbfmt (format, args, gdbfmt);
   fputs_maybe_filtered (linebuffer.c_str (), stream, filter);
 }
 
@@ -2054,13 +2075,19 @@ vfprintf_maybe_filtered (struct ui_file *stream, const char *format,
 void
 vfprintf_filtered (struct ui_file *stream, const char *format, va_list args)
 {
-  vfprintf_maybe_filtered (stream, format, args, 1);
+  vfprintf_maybe_filtered (stream, format, args, true, true);
+}
+
+static void
+vfprintf_filtered_no_gdbfmt (struct ui_file *stream, const char *format, va_list args)
+{
+  vfprintf_maybe_filtered (stream, format, args, true, false);
 }
 
 void
 vfprintf_unfiltered (struct ui_file *stream, const char *format, va_list args)
 {
-  std::string linebuffer = string_vprintf (format, args);
+  std::string linebuffer = string_vprintf_maybe_gdbfmt (format, args, true);
   if (debug_timestamp && stream == gdb_stdlog)
     {
       using namespace std::chrono;
@@ -2087,7 +2114,7 @@ vfprintf_unfiltered (struct ui_file *stream, const char *format, va_list args)
 void
 vprintf_filtered (const char *format, va_list args)
 {
-  vfprintf_maybe_filtered (gdb_stdout, format, args, 1);
+  vfprintf_maybe_filtered (gdb_stdout, format, args, true, false);
 }
 
 void
@@ -2158,6 +2185,17 @@ vfprintf_styled (struct ui_file *stream, const ui_file_style &style,
   set_output_style (stream, ui_file_style ());
 }
 
+/* See utils.h.  */
+
+void
+vfprintf_styled_no_gdbfmt (struct ui_file *stream, const ui_file_style &style,
+			   const char *format, va_list args)
+{
+  set_output_style (stream, style);
+  vfprintf_filtered_no_gdbfmt (stream, format, args);
+  set_output_style (stream, ui_file_style ());
+}
+
 void
 printf_filtered (const char *format, ...)
 {
diff --git a/gdb/utils.h b/gdb/utils.h
index bb73d4a8e72..61b7b5e3bb3 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -435,6 +435,13 @@ extern void vfprintf_styled (struct ui_file *stream,
 			     va_list args)
   ATTRIBUTE_PRINTF (3, 0);
 
+/* Like vfprintf_styled, but do not process gdb-specific format
+   specifiers.  */
+extern void vfprintf_styled_no_gdbfmt (struct ui_file *stream,
+				       const ui_file_style &style,
+				       const char *fmt, va_list args)
+  ATTRIBUTE_PRINTF (3, 0);
+
 /* Like fputs_filtered, but styles the output according to STYLE, when
    appropriate.  */
 
-- 
2.14.5

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

[Pedro Alves]

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.

- See about implementing support for some ui field type other than
  integers, to make sure that we're not missing something.

I'll send some patches for both of these things in reply to this
email.

Thanks,
Pedro Alves

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

[Pedro Alves]

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

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro>  static inline styled_string_s *
Pedro>  styled_string (const ui_file_style &style, const char *str,
Pedro>                styled_string_s &&tmp = {})

Nice trick -- I hadn't seen that one before!

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

Yes, I think so.  It removes some clutter.  The expense is that the call
is tricky and can't be relied on outside of an argument list, really;
but on the other hand I don't expect these things to be used elsewhere.

Tom

[users/palves/format_strings] Down with .ptr() (Re: ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output))

[Pedro Alves]

On 7/1/19 6:26 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro>  static inline styled_string_s *
> Pedro>  styled_string (const ui_file_style &style, const char *str,
> Pedro>                styled_string_s &&tmp = {})
> 
> Nice trick -- I hadn't seen that one before!
> 
> Pedro> Here's a patch implementing the idea above.  I wrote this a couple
> Pedro> weeks ago, and at the time I felt more strongly about it.  Is this
> Pedro> worth it?
> 
> Yes, I think so.  It removes some clutter.  The expense is that the call
> is tricky and can't be relied on outside of an argument list, really;
> but on the other hand I don't expect these things to be used elsewhere.

Yeah, if we ever need to instantiate one of these outside an argument
list, we can still instantiate a styled_string_s instead of
calling styled_string.  But I also don't expect this to be ever necessary.

Alright, I merged this one to users/palves/format_strings.

Thanks,
Pedro Alves

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

[Pedro Alves]

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(-)

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
-- 
2.14.5

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

[Pedro Alves]

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

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

[Tom Tromey]

>>>>> "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

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

[Tom Tromey]

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

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

[Pedro Alves]

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

[users/palves/format_strings] Document the gdb-specific formatters

[Pedro Alves]

On 7/1/19 7:56 PM, Pedro Alves wrote:
> 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.

Like so.  I've merged this.

From c36205f0ccc36ac47b0191c02ceb5c06459ebea3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 1 Jul 2019 19:23:07 +0100
Subject: [PATCH] Document the gdb-specific formatters

---
 gdb/ui-out.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/utils.h  |  5 ++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 67c3962654c..819038ac282 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -189,8 +189,54 @@ class ui_out
 
   void spaces (int numspaces);
   void text (const char *string);
+
+  /* Output a printf-style formatted string.  In addition to the usual
+     printf format specs, this supports a few GDB-specific
+     formatters:
+
+     - '%pF' - output a field.
+
+       The argument is a field, wrapped in any of the base_field_s
+       subclasses.  int_field for integer fields, styled_field for
+       string fields.  This is preferred over separate
+       uiout->field_int(), uiout_>field_string() etc. calls when the
+       formatted message is translatable.  E.g.:
+
+         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));
+
+     - '%p[' - output the following text in a specified style.
+       '%p]' - output the following text in the default style.
+
+       The argument to '%p[' is a ui_file_style pointer.  The argument
+       to '%p]' must be nullptr.
+
+       This is useful when you want to output some portion of a string
+       literal in some style.  E.g.:
+
+	 uiout->message (_(" %p[<repeats %u times>%p]"),
+			 metadata_style.style ().ptr (),
+			 reps, repeats, nullptr);
+
+     - '%ps' - output a styled string.
+
+       The argument is the result of a call to styled_string.  This is
+       useful when you want to output some runtime-generated string in
+       some style.  E.g.:
+
+	 uiout->message (_("this is a target address %ps.\n"),
+			 styled_string (address_style.style (),
+					paddress (gdbarch, pc)));
+
+     Note that these all "abuse" the %p printf format spec, in order
+     to be compatible with GCC's printf format checking.  This is OK
+     because code in GDB that wants to print a host address should use
+     host_address_to_string instead of %p.  */
   void message (const char *format, ...) ATTRIBUTE_PRINTF (2, 3);
   void vmessage (const char *format, va_list args) ATTRIBUTE_PRINTF (2, 0);
+
   void wrap_hint (const char *identstring);
 
   void flush ();
diff --git a/gdb/utils.h b/gdb/utils.h
index 61b7b5e3bb3..d79faf28af3 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -349,7 +349,10 @@ extern struct ui_file *gdb_stdtargin;
 extern void set_screen_width_and_height (int width, int height);
 
 /* More generic printf like operations.  Filtered versions may return
-   non-locally on error.  */
+   non-locally on error.  As an extension over plain printf, these
+   support some GDB-specific format specifiers.  Particularly useful
+   here are the styling formatters: '%p[', '%p]' and '%ps'.  See
+   ui_out::message for details.  */
 
 extern void fputs_filtered (const char *, struct ui_file *);
 
-- 
2.14.5

[users/palves/format_strings] Introduce string_field

[Pedro Alves]

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

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Well, you used printf_filtered the new patches, but that doesn't work
Pedro> with the new format specifiers, yet.  :-)

Oops.  I meant not to do that, but of course made a mistake.

Pedro> If we want the new new formatters to work with printf_(un)filtered,
Pedro> the patch below fixes it by making printf_(un)filtered defer to
Pedro> cli_ui_out::message for format processing, instead of to
Pedro> string_printf (and thus to vsnprintf).  It's maybe a bit roundabout, but
Pedro> I couldn't think of a better/easier way.

We should probably do this, just to avoid future mistakes along these
same lines.

Tom

[users/palves/format_strings] Make printf_filtered support the gdb-specific formatters too

[Pedro Alves]

On 7/1/19 6:42 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Well, you used printf_filtered the new patches, but that doesn't work
> Pedro> with the new format specifiers, yet.  :-)
> 
> Oops.  I meant not to do that, but of course made a mistake.
> 
> Pedro> If we want the new new formatters to work with printf_(un)filtered,
> Pedro> the patch below fixes it by making printf_(un)filtered defer to
> Pedro> cli_ui_out::message for format processing, instead of to
> Pedro> string_printf (and thus to vsnprintf).  It's maybe a bit roundabout, but
> Pedro> I couldn't think of a better/easier way.
> 
> We should probably do this, just to avoid future mistakes along these
> same lines.
> 

I merged this one to the branch as well.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 6/7/19 7:27 PM, Tom Tromey wrote:
> Tom> My patch also removes (really just comments out) %pS and %pN in favor of
> Tom> requiring %ps.  Those could be restored with a bit of effort I guess.
> 
> I think we do need these.
> 
> I have been trying a patch to style gdb's meta-data output, stuff like
> <unavailable>.  There are cases where the <...> contains some other
> value, like:
> 
> 	  fprintf_filtered (stream, " <repeats %u times>", reps);
> 
> Here we want to rewrite as:
> 
> 	  fprintf_filtered (stream, " %pS<repeats %u times>%pN",
>                             metadata_style.style ().ptr (), reps, nullptr);
> 

Do you still see value in keeping %pN?  If we make nullptr mean
"keep the default", as you mentioned earlier, then the above can be
rewritten as:

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

Thanks,
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Do you still see value in keeping %pN?  If we make nullptr mean
Pedro> "keep the default", as you mentioned earlier, then the above can be
Pedro> rewritten as:

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

I was thinking that we'd change the spelling to some form of brackets,
in which case it would be good.  So like "%p[<repeats %u times>%p]".
We discussed this before but I don't recall whether there was some
counter-argument to it.

If we're just using letters, then there doesn't seem to be a reason to
have two %p suffixes.

Tom

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

[Pedro Alves]

On 7/1/19 1:25 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Do you still see value in keeping %pN?  If we make nullptr mean
> Pedro> "keep the default", as you mentioned earlier, then the above can be
> Pedro> rewritten as:
> 
> Pedro> 	  fprintf_filtered (stream, " %pS<repeats %u times>%pS",
> Pedro>                             metadata_style.style ().ptr (), reps, nullptr);
> 
> I was thinking that we'd change the spelling to some form of brackets,
> in which case it would be good.  So like "%p[<repeats %u times>%p]".
> We discussed this before but I don't recall whether there was some
> counter-argument to it.
> 
> If we're just using letters, then there doesn't seem to be a reason to
> have two %p suffixes.

Ah, my counter-argument which I guess I may have never expressed was that I
liked the idea of making %pS+nullptr mean "default", for eliminating
the having to explain/think about the "must pass a nullptr to %pN" wart.

The visual balance of brackets is appealing as well, so I can't say
I have a strong preference either way.  If you've been converting things
already, you'll have a better judgment, so I'll defer to you.
(And it should go without saying, but, to everyone else, you're more
than welcome to voice your opinions too, of course.)

If we went the always-%pS way, I had written the patch and it'd look like this:

From 74899ceab18b23e45225d5560033993596c2a942 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 7 Jun 2019 22:42:57 +0100
Subject: [PATCH] Eliminate %pN

---
 gdb/common/format.c |  1 -
 gdb/ui-out.c        | 13 ++++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/common/format.c b/gdb/common/format.c
index 177f79afee3..6fcbaba290d 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -259,7 +259,6 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 		  case 's':
 		  case 'S':
 		  case 'F':
-		  case 'N':
 		    f++;
 		    break;
 		  }
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index d50fb3a3e3f..25efa98d56a 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -618,11 +618,14 @@ ui_out::vmessage (const char *format, va_list args)
 	      }
 	      break;
 	    case 'S':
-	      style = *va_arg (args, const ui_file_style *);
-	      break;
-	    case 'N':
-	      va_arg (args, void *);
-	      style = {};
+	      {
+		const ui_file_style *style_p
+		  = va_arg (args, const ui_file_style *);
+		if (style_p != nullptr)
+		  style = *style_p;
+		else
+		  style = {};
+	      }
 	      break;
 	    default:
 	      call_do_message (style, current_substring, va_arg (args, void *));
-- 
2.14.5

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> The visual balance of brackets is appealing as well, so I can't say
Pedro> I have a strong preference either way.  If you've been converting things
Pedro> already, you'll have a better judgment, so I'll defer to you.

I haven't really converted much, just what you saw on the branch.

I suspect using some kind of paired brackets will make it a little
harder to forget to close the style.  But probably only a little.  Also
I guess we could add asserts to check that the closing parameter is
always null.

Pedro> If we went the always-%pS way, I had written the patch and it'd
Pedro> look like this:

This is fine by me too.

Tom

[users/palves/format_strings] %pS/%pN -> %p[/%p]

[Pedro Alves]

On 7/1/19 6:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> The visual balance of brackets is appealing as well, so I can't say
> Pedro> I have a strong preference either way.  If you've been converting things
> Pedro> already, you'll have a better judgment, so I'll defer to you.
> 
> I haven't really converted much, just what you saw on the branch.
> 
> I suspect using some kind of paired brackets will make it a little
> harder to forget to close the style.  But probably only a little.  Also
> I guess we could add asserts to check that the closing parameter is
> always null.

Let's try this.  I pushed the patch below to the branch.

From 5984c619b7ee91e6414b1a56d0a735240fb5afce Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 1 Jul 2019 19:07:44 +0100
Subject: [PATCH] %pS/%pN -> %p[/%p]

---
 gdb/common/format.c |  3 ++-
 gdb/ui-out.c        | 12 ++++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/gdb/common/format.c b/gdb/common/format.c
index 177f79afee3..345bdfc4cca 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -259,7 +259,8 @@ format_pieces::format_pieces (const char **arg, bool gdb_extensions)
 		  case 's':
 		  case 'S':
 		  case 'F':
-		  case 'N':
+		  case '[':
+		  case ']':
 		    f++;
 		    break;
 		  }
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index d50fb3a3e3f..065a9209656 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -617,12 +617,16 @@ ui_out::vmessage (const char *format, va_list args)
 		call_do_message (ss->style (), "%s", ss->str ());
 	      }
 	      break;
-	    case 'S':
+	    case '[':
 	      style = *va_arg (args, const ui_file_style *);
 	      break;
-	    case 'N':
-	      va_arg (args, void *);
-	      style = {};
+	    case ']':
+	      {
+		void *arg = va_arg (args, void *);
+		gdb_assert (arg == nullptr);
+
+		style = {};
+	      }
 	      break;
 	    default:
 	      call_do_message (style, current_substring, va_arg (args, void *));
-- 
2.14.5

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

[Philippe Waroquiers]

On Mon, 2019-07-01 at 06:25 -0600, Tom Tromey wrote:
> > > > > > "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Do you still see value in keeping %pN?  If we make nullptr mean
> Pedro> "keep the default", as you mentioned earlier, then the above can be
> Pedro> rewritten as:
> 
> Pedro> 	  fprintf_filtered (stream, " %pS<repeats %u times>%pS",
> Pedro>                             metadata_style.style ().ptr (), reps, nullptr);
> 
> I was thinking that we'd change the spelling to some form of brackets,
> in which case it would be good.  So like "%p[<repeats %u times>%p]".
> We discussed this before but I don't recall whether there was some
> counter-argument to it.
(Arriving late in the discussion ...)

If we have some markers/brackets in the format string to apply style,
why do we keep the style as an additional parameter of the 'printf' like functions ?

E.g. when looking at pango markup, changing the 'style' is done inside the string,
such as :
 "<span foreground=red>some string with red foreground</span>"

So, for GDB, we could have something like:
  
   some_output_function ("The filename is <style=filename>%s</style>.", some_filename);

where the low level of some_output_function would translate the <style=..> into
the real output of the control characters to do the styling.

The advantage of this approach is that the styling can be added for example
in the doc strings either statically and/or built dynamically
(think for example to the new option framework that builds a part of the doc
string:  we might e.g. put in bold the part of the option that is 'unique').

Philippe

> 
> If we're just using letters, then there doesn't seem to be a reason to
> have two %p suffixes.
> 
> To

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

[Tom Tromey]

Philippe> If we have some markers/brackets in the format string to apply
Philippe> style, why do we keep the style as an additional parameter of
Philippe> the 'printf' like functions ?

Philippe> E.g. when looking at pango markup, changing the 'style' is done inside the string,
Philippe> such as :
Philippe>  "<span foreground=red>some string with red foreground</span>"

Philippe> So, for GDB, we could have something like:
  
Philippe>    some_output_function ("The filename is <style=filename>%s</style>.", some_filename);

Philippe> where the low level of some_output_function would translate the <style=..> into
Philippe> the real output of the control characters to do the styling.

Philippe> The advantage of this approach is that the styling can be added for example
Philippe> in the doc strings either statically and/or built dynamically
Philippe> (think for example to the new option framework that builds a part of the doc
Philippe> string:  we might e.g. put in bold the part of the option that is 'unique').

I hadn't really given it much thought.

I suppose the main thing is that the lower levels -- ui-out and ui-file
-- are used for all output in gdb.  So if we had some kind of markup in
the output, we'd need to find all the places emitting unpredictable
output (say, file names or data from the inferior) and arrange for those
to quote the output somehow, to prevent unwanted effects.

On the other hand the programmatic approach doesn't require this, and is
closer to what gdb already does.

Nothing precludes us from doing this later on.  For instance, it could
be done for "help" without touching the rest of gdb.

Tom