Style "pwd" output

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

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
  

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

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