Re: [PATCH] [gdb/cli] Allow source-highlight to autodetect language

[Guinevere Larsen <blarsen@redhat.com>]

On 20/10/2023 14:52, Tom de Vries wrote:
> On 10/20/23 13:30, Guinevere Larsen wrote:
>> On 18/10/2023 18:56, Tom de Vries wrote:
>>> Currently when gdb asks the source-highlight library to highlight a 
>>> file, it
>>> tells it what language file to use.
>>>
>>> For instance, if gdb learns from the debug info that the file is 
>>> language_c,
>>> the language file "c.lang" is used.  This mapping is hardcoded in
>>> get_language_name.
>>>
>>> However, if gdb doesn't know what language file to use, it falls 
>>> back to using
>>> python pygments, and in absence of that, unhighlighted source text.
>>>
>>> In the case of python pygments, it autodetects which language to use 
>>> based on
>>> the file name.
>>>
>>> Add the same capability when using the source-highlight library.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Verified that it works by:
>>> - making get_language_name return nullptr for language_c, and
>>> - checking that source-highlight still manages to highlight a hello 
>>> world.
>>>
>>> PR cli/30966
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30966
>>
>> Thanks for working on this! I tested this the way you mentioned and 
>> it works.
>>
>> I only have one small nit for style.
>>
>>> ---
>>>   gdb/source-cache.c | 18 ++++++++++++++++--
>>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
>>> index 99be3334803..92acb100901 100644
>>> --- a/gdb/source-cache.c
>>> +++ b/gdb/source-cache.c
>>> @@ -37,6 +37,7 @@
>>>   #include <sstream>
>>>   #include <srchilite/sourcehighlight.h>
>>>   #include <srchilite/langmap.h>
>>> +#include <srchilite/settings.h>
>>>   #endif
>>>   /* The number of source files we'll cache.  */
>>> @@ -205,8 +206,6 @@ try_source_highlight (std::string &contents 
>>> ATTRIBUTE_UNUSED,
>>>       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
>>> @@ -214,6 +213,9 @@ try_source_highlight (std::string &contents 
>>> ATTRIBUTE_UNUSED,
>>>        conditional compilation in source-cache.h.  */
>>>     static srchilite::SourceHighlight *highlighter;
>>> +  /* The global source highlight language map object.  */
>>> +  static srchilite::LangMap *langmap;
>>> +
>>>     bool styled = false;
>>>     try
>>>       {
>>> @@ -221,6 +223,18 @@ try_source_highlight (std::string &contents 
>>> ATTRIBUTE_UNUSED,
>>>       {
>>>         highlighter = new srchilite::SourceHighlight ("esc.outlang");
>>>         highlighter->setStyleFile ("esc.style");
>>> +
>>> +      const std::string &datadir = 
>>> srchilite::Settings::retrieveDataDir ();
>>> +      langmap = new srchilite::LangMap (datadir, "lang.map");
>>> +    }
>>> +
>>> +      std::string detected_lang;
>>> +      if (lang_name == nullptr)
>>> +    {
>>> +      detected_lang = langmap->getMappedFileNameFromFileName 
>>> (fullname);
>>> +      if (detected_lang.empty ())
>>> +        return false;
>>> +      lang_name = detected_lang.c_str ();
>>
>> Instead of an early exit here, I think I would prefer if it was:
>>
>> if ( !detected_lang.empty ())
>>    lang_name = detected_lang.c_str ();
>>
>> mainly because if something else is changed in this function, this 
>> might cause bugs that would be very hard to track down.
>>
>> Testing locally, I don't see any performance difference with 5000 
>> repeats, and I feel like the easier flow graph could be would be 
>> worth it.
>>
>
> Hi,
>
> thanks for the review.
>
> I think the proposed change means that:
> - we're allowing lang_name to remain nullptr, which
> - then gets converted into an std::string, which
> - throws an exception, which
> - we catch.
> so agreed, that would work.
>
> However, starting c++23, this doesn't compile anymore:
> ...
> #include <string>
>
> int
> main (void)
> {
>   std::string s = nullptr;
>
>   return 0;
> }
> ...
> so I think it means we shouldn't be relying on that.

ah, I wasn't aware of that change in C++23. With that in mind, i think 
the patch is looking good,

Reviewed-By: Guinevere Larsen <blarsen@redhat.com>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers