Re: [PATCH 1/9] gdb: add symbol_search_kind enum for global_symbol_searcher

[Simon Marchi <simon.marchi@efficios.com>]

On 2026-09-30 05:31, Rohr, Stephan wrote:
> Hi Simon,
> 
> see my feedback inlined below.
> 
> Thanks
> Stephan
> 
>> -----Original Message-----
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Sent: Tuesday, 29 September 2026 21:39
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 1/9] gdb: add symbol_search_kind enum for
>> global_symbol_searcher
>>
>> While reading the global_symbol_searcher code, something that was not
>> clear to me was that despite accepting a domain_search_flags,  searches
> 
> Double spaces in ",  searches".

Done.

>> are mutually exclusive between types, variables and functions.
>>
>> One snippet of code that really confused me was (now removed by a
>> previous commit):
>>
> 
> Should we mention the previous commit " gdb: Skip useless minsym scan in
> variable-only symbol searches" explicitly? It may help future readers of the
> patch.

Done.

>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 27070f74f83e..708df078e2ea 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4698,11 +4698,31 @@ treg_matches_sym_type_name (const
>> compiled_regex &treg,
>>    return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
>>  }
>>
>> +/* Return the domain search flags matching symbols of kind KIND.  */
>> +
>> +static domain_search_flags
>> +to_search_flags (symbol_search_kind kind)
> 
> Same name as " to_search_flags (domain_enum domain)" in symtab.h.
> Not sure if this might be confusing.  I don't have a strong opinion here.

Yes, that's the intent, two overloads that do the same job but with
different input.

>> +{
>> +  switch (kind)
>> +    {
>> +    case symbol_search_kind::VARIABLE:
>> +      return SEARCH_VAR_DOMAIN;
>> +    case symbol_search_kind::FUNCTION:
>> +      return SEARCH_FUNCTION_DOMAIN;
>> +    case symbol_search_kind::TYPE:
> 
> Should we add a comment why the " SEARCH_STRUCT_DOMAIN" is
> included?  I think this might be helpful for future readers, e.g.
> "Some languages have a tag namespace.  Search the "STRUCT_DOMAIN", too."

I hesitated to put that clarification on the enumerator itself, but then
thought that the code here would make it clear that
symbol_search_kind::TYPE includes both the bare types and the "struct
domain" types.  given your comment, I would add that clarification on
the enumerator:

  /* Type names, including those with a struct/union/enum prefix (STRUCT
     domain).  */
  TYPE,

>> @@ -5166,20 +5179,20 @@ 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");
>> +	  gdb_assert_not_reached ("invalid symbol_search_kind");
> 
> 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.

>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index 6443216fd7f0..a35449fbeeda 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2586,6 +2586,23 @@ struct symbol_search
>>  				  const symbol_search &sym_b);
>>  };
>>
>> +/* The kind of symbols searched for by global_symbol_searcher.  */
>> +
>> +enum class symbol_search_kind
>> +{
>> +  /* Variables, excluding enum constants.  */
>> +  VARIABLE,
>> +
>> +  /* Functions.  */
>> +  FUNCTION,
>> +
>> +  /* Type names.  */
> 
> IIUC, this covers "SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN" ?
> Should the comment be extended to state this explicitly, e.g., "Type names,
> including struct, union and enum tags." ?

Done, as per the comment above.

Simon