Re: [RFA] Have 'thread|frame apply' style their output.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Thu, 2019-04-25 at 09:46 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> 'thread|frame apply CMD' launches CMD so that CMD output goes to a string_file.
> Philippe> This patch ensures that string_file for such CMD output contains
> Philippe> style escape sequences that 'thread|frame apply' will later on
> Philippe> output on the real terminal, so as to have CMD output properly styled.
> 
> Thanks for doing this.
> 
> Other than some nits, enumerated below, this looks good to me.
Thanks for the review.  Pushed after fixing (for the "const?" comment,
I have added an explanation to clarify).

Philippe

> 
> Philippe> +/* true if the gdb terminal supports styling, and styling is enabled.  */
> Philippe> +static bool
> Philippe> +term_cli_styling (void)
> 
> No need for the "void" here.
> 
> Philippe> +string_file::string_file ()
> Philippe> +{
> Philippe> +  m_term_out = false;
> Philippe> +}
> 
> I think this should just be an inline initializer in the header.
> Then this definition isn't needed at all.
> 
> Philippe> +string_file::string_file (bool term_out)
> Philippe> +{
> Philippe> +  m_term_out = term_out;
> Philippe> +}
> 
> This can also be in the header file.
> Also it should use initializer syntax like
> 
>   : m_term_out (term_out)
> 
> Philippe> +bool
> Philippe> +string_file::term_out ()
> Philippe> +{
> Philippe> +  return m_term_out;
> Philippe> +}
> Philippe> +
> Philippe> +bool
> Philippe> +string_file::can_emit_style_escape ()
> Philippe> +{
> Philippe> +  return m_term_out && term_cli_styling ();
> Philippe> +}
> 
> This should have comments saying /* See ui-file.h.  */
> 
> Philippe> +bool
> Philippe> +stdio_file::can_emit_style_escape ()
> 
> Likewise.  There are some more instances.
> 
> Philippe> +  return this == gdb_stdout
> Philippe> +    && this->isatty ()
> Philippe> +    && term_cli_styling ();
> 
> The expression needs parens and reindentation to GNU style.
> 
> Philippe> +bool
> Philippe> +tee_file::can_emit_style_escape ()
> Philippe> +{
> Philippe> +  return this == gdb_stdout
> Philippe> +    && m_one->term_out ()
> Philippe> +    && term_cli_styling ();
> 
> Indentation.
> 
> Philippe> +  /* true indicates terminal output behaviour such as cli_styling.  */
> Philippe> +  virtual bool term_out ()
> Philippe> +  { return isatty (); }
> Philippe> +
> Philippe> +  /* true if ANSI escapes can be used on STREAM.  */
> Philippe> +  virtual bool can_emit_style_escape ()
> Philippe> +  { return false; }
> 
> Should these be const?
> 
> Philippe> +  /* If TERM_OUT, construct a string_file with terminal output behaviour
> Philippe> +     such as cli_styling)
> Philippe> +     else collect 'raw' output like the previous constructor.  */
> Philippe> +  string_file (bool term_out);
> 
> Single-argument constructors should nearly always be "explicit".
> 
> Tom