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

[Tom de Vries <tdevries@suse.de>]

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