Re: [PATCH v2 03/11] gdb: factor out regexp compilation from global_symbol_searcher::search

[Andrew Burgess <aburgess@redhat.com>]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Factor out the scopes that compile the regexps, in order to make
> global_symbol_searcher::search itself simpler to read.  Factor out a helper
> to compile a symbol search regex as well.

Three super minor nits, two of which are pre-existing, see inline...

> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index aae82218b5cf..49e8719f4ec0 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4938,66 +4938,86 @@ global_symbol_searcher::add_matching_msymbols
>    return true;
>  }
>  
> -/* See symtab.h.  */
> +/* Compile REGEXP, a symbol name or symbol type regexp.
>  
> -std::vector<symbol_search>
> -global_symbol_searcher::search () const
> +   If REGEXP is nullptr, return an empty optional.  */
> +
> +static std::optional<compiled_regex>
> +compile_symbol_search_regex (const char *regexp)
>  {
> -  std::optional<compiled_regex> name_regex;
> -  std::optional<compiled_regex> type_regex;
> +  if (regexp == nullptr)
> +    return {};
>  
> -  if (m_symbol_name_regexp != NULL)
> -    {
> -      const char *symbol_name_regexp = m_symbol_name_regexp;
> -      std::string symbol_name_regexp_holder;
> +  int cflags
> +    = REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0);
>  
> -      /* Make sure spacing is right for C++ operators.
> -	 This is just a courtesy to make the matching less sensitive
> -	 to how many spaces the user leaves between 'operator'
> -	 and <TYPENAME> or <OPERATOR>.  */
> -      const char *op_end;
> -      const char *opname = operator_chars (symbol_name_regexp, &op_end);
> +  return std::make_optional<compiled_regex> (regexp, cflags,
> +					    _("Invalid regexp"));

I think the indentation of the "Invalid regexp" line is off.

> +}
>  
> -      if (*opname)
> -	{
> -	  int fix = -1;		/* -1 means ok; otherwise number of
> -				    spaces needed.  */
> +/* See symtab.h.  */
>  
> -	  if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> -	    {
> -	      /* There should 1 space between 'operator' and 'TYPENAME'.  */
> -	      if (opname[-1] != ' ' || opname[-2] == ' ')
> -		fix = 1;
> -	    }
> -	  else
> -	    {
> -	      /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
> -	      if (opname[-1] == ' ')
> -		fix = 0;
> -	    }
> -	  /* If wrong number of spaces, fix it.  */
> -	  if (fix >= 0)
> -	    {
> -	      symbol_name_regexp_holder
> -		= string_printf ("operator%.*s%s", fix, " ", opname);
> -	      symbol_name_regexp = symbol_name_regexp_holder.c_str ();
> -	    }
> -	}
> +std::optional<compiled_regex>
> +global_symbol_searcher::compile_name_regex () const
> +{
> +  if (m_symbol_name_regexp == nullptr)
> +    return {};
>  
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      name_regex.emplace (symbol_name_regexp, cflags,
> -			  _("Invalid regexp"));
> -    }
> +  const char *symbol_name_regexp = m_symbol_name_regexp;
> +  std::string symbol_name_regexp_holder;
> +
> +  /* Make sure spacing is right for C++ operators.
> +     This is just a courtesy to make the matching less sensitive
> +     to how many spaces the user leaves between 'operator'
> +     and <TYPENAME> or <OPERATOR>.  */
> +  const char *op_end;
> +  const char *opname = operator_chars (symbol_name_regexp, &op_end);
>  
> -  if (m_symbol_type_regexp != NULL)
> +  if (*opname)
>      {
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      type_regex.emplace (m_symbol_type_regexp, cflags,
> -			  _("Invalid regexp"));
> +      /* -1 means ok; otherwise number of spaces needed.  */
> +      int fix = -1;
> +
> +      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> +	{
> +	  /* There should 1 space between 'operator' and 'TYPENAME'.  */

This comment has a (pre-existing) missing 'be':

  /* There should BE 1 space between .... */

> +	  if (opname[-1] != ' ' || opname[-2] == ' ')
> +	    fix = 1;
> +	}
> +      else
> +	{
> +	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */

Same missing 'BE' here.

With those all fixed:

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

Thanks,
Andrew