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

Tom de Vries
2023-10-16T07:07:20Z

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