Re: [PATCH v2] Factor out the common code in lookup_{static,global}_symbol

[Simon Marchi <simark@simark.ca>]

On 2019-08-05 11:33 a.m., Christian Biesinger via gdb-patches wrote:
> @@ -2614,6 +2576,9 @@ struct global_sym_lookup_data
>    /* The domain to use for our search.  */
>    domain_enum domain;
>  
> +  /* The block index in which to search */

Period and two spaces:

  /* The block index in which to search.  */

> -struct block_symbol
> -lookup_global_symbol (const char *name,
> -		      const struct block *block,
> -		      const domain_enum domain)
> +static struct block_symbol
> +lookup_global_symbol_internal (const char *name,
> +			       enum block_enum block_index,
> +			       const struct block *block,
> +			       const domain_enum domain)

I'd rename that to lookup_symbol_internal, since it's used both in lookup_global_symbol and
lookup_static_symbol.  If you think it's too general (the function doesn't look up local symbols),
Maybe "lookup_global_or_static_symbol"?  And global_sym_lookup_data should be renamed accordingly.

And since BLOCK is only used for looking up an objfile in the global case, I would suggest
making this function accept the objfile instead of a block.  Otherwise, it could be confused
as "lookup symbol in this block".  Also, if some code already has the objfile handy, they can
pass it down, in which case we avoid the cost of lookup_objfile_from_block.

> +/* See symtab.h.  */
> +
> +struct block_symbol
> +lookup_static_symbol (const char *name, const domain_enum domain)
> +{
> +  /* TODO: Should static symbol lookup start with a block as well, so we can
> +     prefer a static symbol in the current CU?  */
> +  return lookup_global_symbol_internal (name, STATIC_BLOCK, nullptr, domain);

I'd say that we can add another argument when the need comes, it's not a stable API.

Simon