Re: [PATCH v2 2/3] gdb: Rename fputs_unfiltered to ui_file_puts.

[Joel Brobecker <brobecker@adacore.com>]

Hi Iain,

> This patch redefines fputs_unfiltered in utils.c, with new behavior to forward parameters to fputs_maybe_filtered.  This makes fputs_unfiltered identical to fputs_filtered, except filtering is disabled.
> 
> Some callers of fputs_unfiltered have been updated to use ui_file_puts where they were using other ui_file_* functions anyway for IO.
> 
> This fixes the problem I saw with \032\032post-prompt annotation being flushed to stdout in the wrong order.
> 
> --
> Iain
> 
> ---
> gdb/ChangeLog:
> 
> 2019-11-29  Iain Buclaw  <ibuclaw@gdcproject.org>
> 
>         * gdb/remote-sim.c (gdb_os_write_stderr): Update.
>         * gdb/remote.c (remote_console_output): Update.
>         * gdb/ui-file.c (fputs_unfiltered): Rename to...
>         (ui_file_puts): ...this.
>         * gdb/ui-file.h (ui_file_puts): Add declaration.
>         * gdb/utils.c (emit_style_escape): Update.
>         (flush_wrap_buffer): Update.
>         (fputs_maybe_filtered): Update.
>         (fputs_unfiltered): Add function.

As far as I can tell, no one has reviewed this patch. This is a call
for help from other Global Maintainers, as I don't feel qualified
to review it all on my own.

As far as I can tell from reading this patch, it really feels like
you are trying to turn the turn the relationship of filtered vs
unfiltered on its head. Right now, fputs_maybe_filtered is the one
that depends on fputs_unfiltered, and this is realy what feels
natural to me.

For some reason, you decided to do the other way around, but without
a real explanation of why we are getting text being printed in the wrong
order, and why your patch fixes it or why you think this is the best
approach.


> 
> ---
> 
> 

> diff --git a/gdb/remote-sim.c b/gdb/remote-sim.c
> index f9c2f605c3..b7eeccf91b 100644
> --- a/gdb/remote-sim.c
> +++ b/gdb/remote-sim.c
> @@ -378,7 +378,7 @@ gdb_os_write_stderr (host_callback *p, const char *buf, int len)
>      {
>        b[0] = buf[i];
>        b[1] = 0;
> -      fputs_unfiltered (b, gdb_stdtargerr);
> +      ui_file_puts (gdb_stdtargerr, b);
>      }
>    return len;
>  }
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 054802f744..7cc2a0470b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -6782,7 +6782,7 @@ remote_console_output (const char *msg)
>  
>        tb[0] = c;
>        tb[1] = 0;
> -      fputs_unfiltered (tb, gdb_stdtarg);
> +      ui_file_puts (gdb_stdtarg, tb);
>      }
>    ui_file_flush (gdb_stdtarg);
>  }
> diff --git a/gdb/ui-file.c b/gdb/ui-file.c
> index d7d113856e..6a3ebfb6a9 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -150,7 +150,7 @@ ui_file_read (struct ui_file *file, char *buf, long length_buf)
>  }
>  
>  void
> -fputs_unfiltered (const char *buf, struct ui_file *file)
> +ui_file_puts (struct ui_file *file, const char *buf)
>  {
>    file->puts (buf);
>  }
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index 711a888a2e..8b57622a09 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -112,6 +112,8 @@ extern void ui_file_write_async_safe (struct ui_file *file, const char *buf,
>  
>  extern long ui_file_read (struct ui_file *file, char *buf, long length_buf);
>  
> +extern void ui_file_puts (struct ui_file *file, const char *buf);
> +
>  extern int gdb_console_fputs (const char *, FILE *);
>  
>  /* A std::string-based ui_file.  Can be used as a scratch buffer for
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 5d6f680bce..0e09f646bf 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1405,7 +1405,7 @@ emit_style_escape (const ui_file_style &style,
>    if (stream == nullptr)
>      wrap_buffer.append (style.to_ansi ());
>    else
> -    fputs_unfiltered (style.to_ansi ().c_str (), stream);
> +    ui_file_puts (stream, style.to_ansi ().c_str ());
>  }
>  
>  /* Set the current output style.  This will affect future uses of the
> @@ -1539,7 +1539,7 @@ flush_wrap_buffer (struct ui_file *stream)
>  {
>    if (stream == gdb_stdout && !wrap_buffer.empty ())
>      {
> -      fputs_unfiltered (wrap_buffer.c_str (), stream);
> +      ui_file_puts (stream, wrap_buffer.c_str ());
>        wrap_buffer.clear ();
>      }
>  }
> @@ -1695,7 +1695,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>        || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
>      {
>        flush_wrap_buffer (stream);
> -      fputs_unfiltered (linebuffer, stream);
> +      ui_file_puts (stream, linebuffer);
>        return;
>      }
>  
> @@ -1795,7 +1795,7 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
>  	      /* Now output indentation and wrapped string.  */
>  	      if (wrap_column)
>  		{
> -		  fputs_unfiltered (wrap_indent, stream);
> +		  ui_file_puts (stream, wrap_indent);
>  		  if (stream->can_emit_style_escape ())
>  		    emit_style_escape (save_style, stream);
>  		  /* FIXME, this strlen is what prevents wrap_indent from
> @@ -1833,6 +1833,12 @@ fputs_filtered (const char *linebuffer, struct ui_file *stream)
>    fputs_maybe_filtered (linebuffer, stream, 1);
>  }
>  
> +void
> +fputs_unfiltered (const char *linebuffer, struct ui_file *stream)
> +{
> +  fputs_maybe_filtered (linebuffer, stream, 0);
> +}
> +
>  /* See utils.h.  */
>  
>  void


-- 
Joel