This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Have 'thread|frame apply' style their output.
- From: Tom Tromey <tom at tromey dot com>
- To: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 25 Apr 2019 09:46:26 -0600
- Subject: Re: [RFA] Have 'thread|frame apply' style their output.
- References: <20190309225932.14568-1-philippe.waroquiers@skynet.be>
>>>>> "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