Re: [patch] Convert frame_stash to a hash table

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I think this patch improves the frame_stash "with filters" without 
Phil> affecting "no filters" performance.

Phil> Tested on x8664 with no regressions.

Phil> What do you think?

Thanks, Phil.  I appreciate this.

A few nits below.

Phil> +/* A frame stash used to speed up frame lookups.  Create a hash table
Phil> +   to stash frames previously accessed from the frame cache for
Phil> +   quicker subsequent retrieval.  The hash table is emptied whenever
Phil> +   the frame cache is invalidated.  */
Phil> +static htab_t frame_stash;

We use a blank line between the intro comment and the definition now.
This applies elsewhere.

Phil> +  if (! f_id.stack_addr_p && ! f_id.code_addr_p && ! f_id.special_addr)
Phil> +    gdb_assert ("Cannot calculate hash for frame_id.");

This assert will never fail, since its argument is true.
You want:

gdb_assert (f_id.stack_addr_p || f_id.code_addr_p || f_id.special_addr_p);

Also note the missing "_p" from special_addr_p in the patch.

Phil> +  if (f_id.stack_addr_p == 1)

These are booleans so don't compare against 1.

Phil> +      slot = (struct frame_info **) htab_find_slot (frame_stash,
Phil> +						    frame,
Phil> +						    INSERT);
Phil> +      if (*slot != frame)
Phil> +	*slot = frame;

I think you might as well assign unconditionally.
Otherwise I think readers will wonder when this can happen.
But, I think it can't happen.
Alternatively, if it can happen, then we need a comment.

thanks,
Tom