Re: [PATCH v2 01/11] gdb: add symbol_search_kind enum for global_symbol_searcher

[Simon Marchi <simon.marchi@polymtl.ca>]

On 10/1/26 6:30 AM, Andrew Burgess wrote:
> 
> I think this is looking good.  Just one minor comment inline below:
> 
> simon.marchi@polymtl.ca writes:
> 
>> @@ -5166,20 +5179,18 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
>>        const char *classname;
>>        switch (kind)
>>  	{
>> -	case VAR_DOMAIN:
>> +	case symbol_search_kind::VARIABLE:
>>  	  classname = "variable";
>>  	  break;
>> -	case FUNCTION_DOMAIN:
>> +	case symbol_search_kind::FUNCTION:
>>  	  classname = "function";
>>  	  break;
>> -	case TYPE_DOMAIN:
>> +	case symbol_search_kind::TYPE:
>>  	  classname = "type";
>>  	  break;
>> -	case MODULE_DOMAIN:
>> +	case symbol_search_kind::MODULE:
>>  	  classname = "module";
>>  	  break;
>> -	default:
>> -	  gdb_assert_not_reached ("invalid domain enum");
>>  	}
> 
> I initially wrote: please don't drop the default case, but then I
> checked the V1 review and saw this change was made specifically to
> address the feedback there.
> 
> What I don't understand is you said:
> 
>>> I think we could drop the "default" case as part of this refactoring.  All
>>> cases are handled; we lose compile time warnings for unhandled cases with
>>> the "default" case.
>>
>> Agreed that we can drop the default.  Note that we don't build with
>> -Wswitch, but we specifically enable it locally here and there.
> 
> If the warning is mostly off, including here, then deleting the default
> seems like the wrong choice.
> 
> I'm not too fussed, I think there's a middle group, that would keep my
> happy:
> 
>  1. initialise classname to nullptr, and
> 
>  2. follow this switch statement with:
>       gdb_assert (classname != nullptr);

You're right, I applied the suggestion without thinking too much about
it.  We definitely want an assert to fire instead of printing from an
uninitialized pointer.  If that same code was in a function of its own,
I would have done it like this:

static const char *
ssk_to_string (symbol_search_kind kind)
{
  switch (kind)
    {
    case symbol_search_kind::VARIABLE:
      return "variable";

    case symbol_search_kind::FUNCTION:
      return "function";

    case symbol_search_kind::TYPE:
      return "type";

    case symbol_search_kind::MODULE:
      return "module";
    }

  gdb_assert_not_reached ("invalid symbol_search_kind value");
}

There is even an example of that in the same patch (the to_search_flags
function).

I'll initialize the variable with nullptr and add a nullptr check after
the switch.  I prefer this over the default, because if we ever enable
-Wswitch (which will probably never happen, I tried it before and it was
painful), then we'll get a compile-time error if we are missing a case.

I don't think that this code is duplicated anywhere, so I don't think
it's worth moving it to a separate function.

> 
> Even with the warning on it doesn't protect us from random garbage being
> cast to type symbol_search_kind and then passed to this function, which
> the 'default:' case does protect against.  But the trailing assert is
> probably better anyway as it ensures each switch case is doing what
> needs to be done.
> 
> With that:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon