This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Convert frame_stash to a hash table
- From: Tom Tromey <tromey at redhat dot com>
- To: Phil Muldoon <pmuldoon at redhat dot com>
- Cc: "gdb-patches\ at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 16 May 2013 07:42:52 -0600
- Subject: Re: [patch] Convert frame_stash to a hash table
- References: <5194DA88 dot 6020705 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