Re: [PATCH] gdb/styling: only check TERM environment once, during initialisation

[Kevin Buettner <kevinb@redhat.com>]

On Wed, 19 Feb 2025 20:31:04 +0000
Andrew Burgess <aburgess@redhat.com> wrote:

> We currently check the TERM environment variable any time we call one
> of the ui_file::can_emit_style_escape() functions.  This seems
> excessive.
> 
> During GDB's startup we also check for the NO_COLOR environment
> variable and disable styling if this is set.
> 
> I propose that we combine these two checks, and perform them just once
> during startup (as the current NO_COLOR check is currently done).  As
> with the NO_COLOR check, if the TERM variable is set to "dumb"
> indicating that styling is not supported then we should just set
> cli_styling to false.
> 
> This does mean that the user can then 'set style enabled on', even for
> a dumb terminal, which was not possible previously.  Before this
> commit the "dumb" terminal check was separate and would prevent
> styling even if 'set style enabled on' was in effect.
> 
> Of course, trying to turn on styling in a dumb terminal might not give
> the results that a user hope for.  And so, I have implemented a check
> in `set_style_enabled`, so in a dumb terminal a user will see this:
> 
>   (gdb) set style enabled on
>   warning: The current terminal doesn't support styling.  Styled output might not appear as expected.
> 
> After which GDB will try to emit styling.  We could, potentially,
> prevent styling being enabled instead of emitting a warning, but I'm
> inclined to let the user turn on styling if they really want to.

This all makes sense to me. 

I found one nit...

> diff --git a/gdb/ui-file.c b/gdb/ui-file.c
> index dd15ea427bc..14931f3aef9 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -180,28 +180,12 @@ null_file::write_async_safe (const char *buf, long sizeof_buf)
>  
>  \f
>  
> -/* true if the gdb terminal supports styling, and styling is enabled.  */
> +/* Return true styling is currently enabled.  */

I think you should add an "if" to the comment, i.e. "Return true if ..."

Approved-by: Kevin Buettner <kevinb@redhat.com>

Kevin