Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Tom

On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote:
> Function source_cache::ensure contains some code using the GNU
> source-highlight library.
> 
> The code is a sizable chunk of the function, and contains conditional
> compilation in a slightly convoluted way:
> ...
>        if (!already_styled)
>  #endif /* HAVE_SOURCE_HIGHLIGHT */
>        {
> ...
> 
> Fix this by factoring out the code into new function try_source_highlight,
> such that:
> - source_cache::ensure is easier to read, and
> - the conditional compilation is at the level of the function body.
> 
> Tested on x86_64-linux.
> ---
>  gdb/source-cache.c | 99 +++++++++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 40 deletions(-)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 7ef54411c69..9757721e932 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -191,6 +191,63 @@ get_language_name (enum language lang)
>  
>  #endif /* HAVE_SOURCE_HIGHLIGHT */
>  
> +/* Try to highlight CONTENTS from file FULLNAME in language LANG using
> +   the GNU source-higlight library.  Return true if highlighting
> +   succeeded.  */
> +
> +static bool
> +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED,
> +		      enum language lang ATTRIBUTE_UNUSED,
> +		      const std::string &fullname ATTRIBUTE_UNUSED)
> +{
> +#ifdef HAVE_SOURCE_HIGHLIGHT
> +  if (!use_gnu_source_highlight)
> +    return false;
> +
> +  const char *lang_name = get_language_name (lang);
> +  if (lang_name == nullptr)
> +    return false;
> +
> +  /* The global source highlight object, or null if one was
> +     never constructed.  This is stored here rather than in
> +     the class so that we don't need to include anything or do
> +     conditional compilation in source-cache.h.  */
> +  static srchilite::SourceHighlight *highlighter;

Even if this is pre-existing code, I think I would be tempted to get the
initialization of HIGHLIGHTER outside of the TRY block (new can throw,
we might not want to hide it).

Using an immediately invoked lambda does the trick and works well with
static variables:

    static srchilite::SourceHighlight *highlighter
      = []()
        {
	  srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang");
	  h->setStyleFile ("esc.style");
	  return h;
	} ();

Best,
Lancelot.

> +
> +  bool styled = false;
> +  try
> +    {
> +      if (highlighter == nullptr)
> +	{
> +	  highlighter = new srchilite::SourceHighlight ("esc.outlang");
> +	  highlighter->setStyleFile ("esc.style");
> +	}
> +
> +      std::istringstream input (contents);
> +      std::ostringstream output;
> +      highlighter->highlight (input, output, lang_name, fullname);
> +#if __cplusplus >= 202002L
> +      contents = std::move (output).str();
> +#else
> +      contents = output.str ();
> +#endif
> +      styled = true;
> +    }
> +  catch (...)
> +    {
> +      /* Source Highlight will throw an exception if
> +	 highlighting fails.  One possible reason it can fail
> +	 is if the language is unknown -- which matters to gdb
> +	 because Rust support wasn't added until after 3.1.8.
> +	 Ignore exceptions here. */
> +    }
> +
> +  return styled;
> +#else
> +  return false;
> +#endif /* HAVE_SOURCE_HIGHLIGHT */
> +}
> +
>  /* See source-cache.h.  */
>  
>  bool
> @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s)
>  
>    if (source_styling && gdb_stdout->can_emit_style_escape ())
>      {
> -#ifdef HAVE_SOURCE_HIGHLIGHT
> -      bool already_styled = false;
> -      const char *lang_name = get_language_name (s->language ());
> -      if (lang_name != nullptr && use_gnu_source_highlight)
> -	{
> -	  /* The global source highlight object, or null if one was
> -	     never constructed.  This is stored here rather than in
> -	     the class so that we don't need to include anything or do
> -	     conditional compilation in source-cache.h.  */
> -	  static srchilite::SourceHighlight *highlighter;
> -
> -	  try
> -	    {
> -	      if (highlighter == nullptr)
> -		{
> -		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
> -		  highlighter->setStyleFile ("esc.style");
> -		}
> -
> -	      std::istringstream input (contents);
> -	      std::ostringstream output;
> -	      highlighter->highlight (input, output, lang_name, fullname);
> -#if __cplusplus >= 202002L
> -	      contents = std::move (output).str();
> -#else
> -	      contents = output.str ();
> -#endif
> -	      already_styled = true;
> -	    }
> -	  catch (...)
> -	    {
> -	      /* Source Highlight will throw an exception if
> -		 highlighting fails.  One possible reason it can fail
> -		 is if the language is unknown -- which matters to gdb
> -		 because Rust support wasn't added until after 3.1.8.
> -		 Ignore exceptions here and fall back to
> -		 un-highlighted text. */
> -	    }
> -	}
> +      bool already_styled
> +	= try_source_highlight (contents, s->language (), fullname);
>  
>        if (!already_styled)
> -#endif /* HAVE_SOURCE_HIGHLIGHT */
>  	{
>  	  gdb::optional<std::string> ext_contents;
>  	  ext_contents = ext_lang_colorize (fullname, contents);
> -- 
> 2.35.3
>