[v2,06/11] gdb: use a switch on m_kind in add_matching_symbols

  From: Simon Marchi <simon.marchi@efficios.com>

add_matching_symbols applies some filters that only make sense for some
kinds of searches: the type regexp for variables and functions, the
LOC_UNRESOLVED / enum constant filter for variables, and the line number
filter for modules.  Some of these check m_kind, others check the
symbol's domain.  Since the earlier sym->matches check only lets through
symbols whose domain corresponds to m_kind, the two are equivalent.

Group these filters in a single switch on m_kind.  Add a comment for
the module filter, which skips module declarations, because it wasn't
obvious to me at first sight.

Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
---
 gdb/symtab.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)
  

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> add_matching_symbols applies some filters that only make sense for some
> kinds of searches: the type regexp for variables and functions, the
> LOC_UNRESOLVED / enum constant filter for variables, and the line number
> filter for modules.  Some of these check m_kind, others check the
> symbol's domain.  Since the earlier sym->matches check only lets through
> symbols whose domain corresponds to m_kind, the two are equivalent.
>
> Group these filters in a single switch on m_kind.  Add a comment for
> the module filter, which skips module declarations, because it wasn't
> obvious to me at first sight.
>
> Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
> ---
>  gdb/symtab.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 35dd2981a53a..6cc85a7eacc5 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4858,25 +4858,30 @@ global_symbol_searcher::add_matching_symbols
>  				       nullptr, 0) != 0)
>  		continue;
>  
> -	      if (((sym->domain () == VAR_DOMAIN
> -		    || sym->domain () == FUNCTION_DOMAIN)
> -		   && type_regex.has_value ()
> -		   && !regex_matches_sym_type_name (*type_regex, sym)))
> -		continue;
> -
> -	      if (m_kind == symbol_search_kind::VARIABLE)
> +	      switch (m_kind)
>  		{
> +		case symbol_search_kind::VARIABLE:
> +		  /* LOC_CONST can be used for more than just enums, e.g.,
> +		     C++ static const members.  We only want to skip enums
> +		     here.  */
>  		  if (sym->loc_class () == LOC_UNRESOLVED
> -		      /* LOC_CONST can be used for more than
> -			 just enums, e.g., c++ static const
> -			 members.  We only want to skip enums
> -			 here.  */
>  		      || (sym->loc_class () == LOC_CONST
> -			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
> +			  && sym->type ()->code () == TYPE_CODE_ENUM))
>  		    continue;
> +		  [[fallthrough]];
> +
> +		case symbol_search_kind::FUNCTION:
> +		  if (type_regex.has_value ()
> +		      && !regex_matches_sym_type_name (*type_regex, sym))
> +		    continue;
> +		  break;
> +
> +		case symbol_search_kind::MODULE:
> +		  /* Skip module declarations, keep only definitions.  */
> +		  if (sym->line () == 0)
> +		    continue;
> +		  break;

I agree with Stephan's suggestion that you should add:

  case symbol_search_kind::TYPE:
    break;

I would not focus on GDB's use or non-use of -Wswitch.  Instead, my
argument is that by specifically mentioning the extra case we make it
clear that nothing needs to be done rather than leaving it for a future
reader of the code to try and figure out if TYPE is missing on purpose,
or by accident.

I don't find the reasoning that there are lots of other missing case
labels compelling, I think the argument for or against should stand on
its own technical merits -- unless you're suggesting that the GDB style
is to trim the case list as much as possible?

That said, I only care enough to send one email on the topic :)  If
you're still not convinced then go ahead:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>  		}
> -	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
> -		continue;
>  
>  	      /* Match, insert if not already in the results.  */
>  	      if (result_set->size () < m_max_search_results)
> -- 
> 2.55.0
  

On 10/1/26 11:01 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> add_matching_symbols applies some filters that only make sense for some
>> kinds of searches: the type regexp for variables and functions, the
>> LOC_UNRESOLVED / enum constant filter for variables, and the line number
>> filter for modules.  Some of these check m_kind, others check the
>> symbol's domain.  Since the earlier sym->matches check only lets through
>> symbols whose domain corresponds to m_kind, the two are equivalent.
>>
>> Group these filters in a single switch on m_kind.  Add a comment for
>> the module filter, which skips module declarations, because it wasn't
>> obvious to me at first sight.
>>
>> Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
>> ---
>>  gdb/symtab.c | 33 +++++++++++++++++++--------------
>>  1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 35dd2981a53a..6cc85a7eacc5 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4858,25 +4858,30 @@ global_symbol_searcher::add_matching_symbols
>>  				       nullptr, 0) != 0)
>>  		continue;
>>  
>> -	      if (((sym->domain () == VAR_DOMAIN
>> -		    || sym->domain () == FUNCTION_DOMAIN)
>> -		   && type_regex.has_value ()
>> -		   && !regex_matches_sym_type_name (*type_regex, sym)))
>> -		continue;
>> -
>> -	      if (m_kind == symbol_search_kind::VARIABLE)
>> +	      switch (m_kind)
>>  		{
>> +		case symbol_search_kind::VARIABLE:
>> +		  /* LOC_CONST can be used for more than just enums, e.g.,
>> +		     C++ static const members.  We only want to skip enums
>> +		     here.  */
>>  		  if (sym->loc_class () == LOC_UNRESOLVED
>> -		      /* LOC_CONST can be used for more than
>> -			 just enums, e.g., c++ static const
>> -			 members.  We only want to skip enums
>> -			 here.  */
>>  		      || (sym->loc_class () == LOC_CONST
>> -			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
>> +			  && sym->type ()->code () == TYPE_CODE_ENUM))
>>  		    continue;
>> +		  [[fallthrough]];
>> +
>> +		case symbol_search_kind::FUNCTION:
>> +		  if (type_regex.has_value ()
>> +		      && !regex_matches_sym_type_name (*type_regex, sym))
>> +		    continue;
>> +		  break;
>> +
>> +		case symbol_search_kind::MODULE:
>> +		  /* Skip module declarations, keep only definitions.  */
>> +		  if (sym->line () == 0)
>> +		    continue;
>> +		  break;
> 
> I agree with Stephan's suggestion that you should add:
> 
>   case symbol_search_kind::TYPE:
>     break;
> 
> I would not focus on GDB's use or non-use of -Wswitch.  Instead, my
> argument is that by specifically mentioning the extra case we make it
> clear that nothing needs to be done rather than leaving it for a future
> reader of the code to try and figure out if TYPE is missing on purpose,
> or by accident.
> 
> I don't find the reasoning that there are lots of other missing case
> labels compelling, I think the argument for or against should stand on
> its own technical merits -- unless you're suggesting that the GDB style
> is to trim the case list as much as possible?

When seeing an empty case, my intuition would be to remove it, but I
really don't have strong feelings about that.  Since you both mentioned
it, I'll add it.

> 
> That said, I only care enough to send one email on the topic :)  If
> you're still not convinced then go ahead:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon