[v2,2/4,gdb/cli] Factor out try_source_highlight

  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(-)
  

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
>
  

On 10/13/23 18:52, Lancelot SIX wrote:
> 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).
> 

Hi,

The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch 
exception when constructing the highlighter"), which does the opposite 
of what you propose here.

And I think the rationale given there still holds.

I suppose if we're interested in not hiding that new throws, you could 
catch the exceptions of interest and handle them appropriately, by say 
issuing a warning or rethrowing.

I will leave this as is for now.

Thanks,
- Tom

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

> 
> Hi,
> 
> The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch
> exception when constructing the highlighter"), which does the opposite of
> what you propose here.
> 
> And I think the rationale given there still holds.
> 

Hi Tom,

Thanks for pointing this out, I missed this commit.

Best,
Lancelot.

> I suppose if we're interested in not hiding that new throws, you could catch
> the exceptions of interest and handle them appropriately, by say issuing a
> warning or rethrowing.
> 
> I will leave this as is for now.
> 
> Thanks,
> - Tom
>