[2/2] Use gdb::unordered_set for Ada symbol cache

  This changes the Ada symbol cache to use gdb::unordered_set rather
than an htab.
---
 gdb/ada-lang.c | 96 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 44 deletions(-)
  

LGTM, see nits below.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

On 4/14/25 4:19 PM, Tom Tromey wrote:
> This changes the Ada symbol cache to use gdb::unordered_set rather
> than an htab.
> ---
>  gdb/ada-lang.c | 96 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 52 insertions(+), 44 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 8e76fd9b7cb2e2a6bf0ae3cfcd33f44c5c36731c..b1df25763f682b87c82951a7bebd9a7a53832a59 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -36,7 +36,7 @@
>  #include "objfiles.h"
>  #include "breakpoint.h"
>  #include "gdbcore.h"
> -#include "hashtab.h"
> +#include "gdbsupport/unordered_map.h"

The include for unordered_map is not needed, since you use
unordered_set.


>  #include "gdbsupport/gdb_obstack.h"
>  #include "ada-lang.h"
>  #include "completer.h"
> @@ -358,28 +358,50 @@ struct cache_entry_search
>    }
>  };

cache_entry_search::hash() can be removed.

>  
> -/* Hash function for cache_entry.  */
> +/* Hash function for cache entry.  */
>  
> -static hashval_t
> -hash_cache_entry (const void *v)
> +struct cache_entry_hash
>  {
> -  const cache_entry *entry = (const cache_entry *) v;
> -  return htab_hash_string (entry->name.c_str ());
> -}
> +  using is_transparent = void;
> +  using is_avalanching = void;
>  
> -/* Equality function for cache_entry.  */
> +  uint64_t operator() (const cache_entry &entry)
> +    const noexcept
> +  {
> +    return ankerl::unordered_dense::hash<std::string> () (entry.name);
> +  }
>  
> -static int
> -eq_cache_entry (const void *a, const void *b)
> +  uint64_t operator() (const cache_entry_search &entry)
> +    const noexcept
> +  {
> +    return ankerl::unordered_dense::hash<std::string_view> () (entry.name);
> +  }
> +};
> +
> +/* Equality function for cache entry.  */
> +
> +struct cache_entry_eq
>  {
> -  const cache_entry *entrya = (const cache_entry *) a;
> -  const cache_entry_search *entryb = (const cache_entry_search *) b;
> +  using is_transparent = void;
>  
> -  return entrya->domain == entryb->domain && entrya->name == entryb->name;
> -}
> +  bool operator() (const cache_entry &lhs, const cache_entry &rhs)
> +    const noexcept
> +  {
> +    return lhs.domain == rhs.domain && lhs.name == rhs.name;
> +  }
> +
> +  uint64_t operator() (const cache_entry_search &lhs, const cache_entry &rhs)
> +    const noexcept
> +  {
> +    return lhs.domain == rhs.domain && lhs.name == rhs.name;
> +  }
> +};

As I mentioned in the past, I would write these in a way that encodes
the actual logic in a single method (the first one would use the second
one).  But I won't object if you prefer it like this, I can see the
argument that it's easier to read this way.

> +
> +using cache_entry_set
> +   = gdb::unordered_set<cache_entry, cache_entry_hash, cache_entry_eq>;
>  
>  /* Key to our per-program-space data.  */
> -static const registry<program_space>::key<htab, htab_deleter>
> +static const registry<program_space>::key<cache_entry_set>
>    ada_pspace_data_handle;
>  
>  /* Return this module's data for the given program space (PSPACE).
> @@ -387,17 +409,12 @@ static const registry<program_space>::key<htab, htab_deleter>
>  
>     This function always returns a valid object.  */
>  
> -static htab_t
> +static cache_entry_set *
>  get_ada_pspace_data (struct program_space *pspace)
>  {
> -  htab_t data = ada_pspace_data_handle.get (pspace);
> +  cache_entry_set *data = ada_pspace_data_handle.get (pspace);
>    if (data == nullptr)
> -    {
> -      data = htab_create_alloc (10, hash_cache_entry, eq_cache_entry,
> -				htab_delete_entry<cache_entry>,
> -				xcalloc, xfree);
> -      ada_pspace_data_handle.set (pspace, data);
> -    }
> +    data = ada_pspace_data_handle.emplace (pspace);

I think this function could return a reference instead of a pointer.
The documentation just above it could also be cleanup up a bit.  For
instance, it no longer makes sense to say "add a zero'ed one" anymore.

>  
>    return data;
>  }
> @@ -4690,19 +4707,18 @@ static int
>  lookup_cached_symbol (const char *name, domain_search_flags domain,
>  		      struct symbol **sym, const struct block **block)
>  {
> -  htab_t tab = get_ada_pspace_data (current_program_space);
> +  cache_entry_set *htab = get_ada_pspace_data (current_program_space);
>    cache_entry_search search;
>    search.name = name;
>    search.domain = domain;
>  
> -  cache_entry *e = (cache_entry *) htab_find_with_hash (tab, &search,
> -							search.hash ());
> -  if (e == nullptr)
> +  auto iter = htab->find (search);
> +  if (iter == htab->end ())
>      return 0;
>    if (sym != nullptr)
> -    *sym = e->sym;
> +    *sym = iter->sym;
>    if (block != nullptr)
> -    *block = e->block;
> +    *block = iter->block;
>    return 1;
>  }
>  
> @@ -4730,21 +4746,13 @@ cache_symbol (const char *name, domain_search_flags domain,
>  	return;
>      }
>  
> -  htab_t tab = get_ada_pspace_data (current_program_space);
> -  cache_entry_search search;
> -  search.name = name;
> -  search.domain = domain;
> -
> -  void **slot = htab_find_slot_with_hash (tab, &search,
> -					  search.hash (), INSERT);
> -
> -  cache_entry *e = new cache_entry;
> -  e->name = name;
> -  e->domain = domain;
> -  e->sym = sym;
> -  e->block = block;
> -
> -  *slot = e;
> +  cache_entry_set *tab = get_ada_pspace_data (current_program_space);
> +  cache_entry e;
> +  e.name = name;
> +  e.domain = domain;
> +  e.sym = sym;
> +  e.block = block;
> +  tab->insert (std::move (e));

You could write:

tab->insert (cache_entry {name, domain, sym, block});

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> As I mentioned in the past, I would write these in a way that encodes
Simon> the actual logic in a single method (the first one would use the second
Simon> one).  But I won't object if you prefer it like this, I can see the
Simon> argument that it's easier to read this way.

I ended up using a template method, which works nicely in this case.

Also doing this pointed out that I'd gotten one of the equality method
return types wrong -- bad cut & paste.

Tom