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

[Tom Tromey <tom@tromey.com>]

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

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