From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79842 invoked by alias); 2 Apr 2018 01:39:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 79833 invoked by uid 89); 2 Apr 2018 01:39:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS,T_FILL_THIS_FORM_SHORT,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Apr 2018 01:39:46 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C5E131E4B2; Sun, 1 Apr 2018 21:39:44 -0400 (EDT) Subject: Re: [RFA 01/10] Remove some cleanups from search_minsyms_for_name To: Tom Tromey , gdb-patches@sourceware.org References: <20180401163539.15314-1-tom@tromey.com> <20180401163539.15314-2-tom@tromey.com> From: Simon Marchi Message-ID: <8f38f8d0-fe2e-7023-dd57-7a32af834e4a@simark.ca> Date: Mon, 02 Apr 2018 01:39:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180401163539.15314-2-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-04/txt/msg00030.txt.bz2 On 2018-04-01 12:35 PM, Tom Tromey wrote: > This changes struct collect_minsyms to use a std::vector, which > enables the removal of a cleanup from search_minsyms_for_name. This > also changes iterate_over_minimal_symbols to take a > gdb::function_view, which makes a function in linespec.c more > type-safe. > > gdb/ChangeLog > 2018-03-31 Tom Tromey > > * minsyms.h (iterate_over_minimal_symbols): Update. > * minsyms.c (iterate_over_minimal_symbols): Take a > gdb::function_view. > * linespec.c (struct collect_minsyms): Add constructor and > initializers. > : Now a std::vector. > (compare_msyms): Now a std::sort comparator. > (add_minsym): Change type of second parameter. > (search_minsyms_for_name): Update. > --- > gdb/ChangeLog | 12 +++++++ > gdb/linespec.c | 112 ++++++++++++++++++++++++++++----------------------------- > gdb/minsyms.c | 9 ++--- > gdb/minsyms.h | 8 ++--- > 4 files changed, 72 insertions(+), 69 deletions(-) > > diff --git a/gdb/linespec.c b/gdb/linespec.c > index 1236b3f475..bd09f57b05 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -46,6 +46,7 @@ > #include "location.h" > #include "common/function-view.h" > #include "common/def-vector.h" > +#include > > /* An enumeration of the various things a user might attempt to > complete for a linespec location. */ > @@ -4375,8 +4376,15 @@ minsym_found (struct linespec_state *self, struct objfile *objfile, > > struct collect_minsyms > { > + collect_minsyms (int funfirstline_, int list_mode_, struct symtab *symtab_) > + : symtab (symtab_), > + funfirstline (funfirstline_), Note, funfirstline is unused, so you might as well remove it. > + list_mode (list_mode_) > + { > + } > + > /* The objfile we're examining. */ > - struct objfile *objfile; > + struct objfile *objfile = nullptr; > > /* Only search the given symtab, or NULL to search for all symbols. */ > struct symtab *symtab; > @@ -4388,7 +4396,7 @@ struct collect_minsyms > int list_mode; > > /* The resulting symbols. */ > - VEC (bound_minimal_symbol_d) *msyms; > + std::vector msyms; > }; I would lean towards removing the collect_minsyms structure and just pass anything that's required by parameter to add_minsym. It's easier to follow the flow of execution when parameters are passed explicitly than through a structure. > > /* A helper function to classify a minimal_symbol_type according to > @@ -4415,47 +4423,43 @@ classify_mtype (enum minimal_symbol_type t) > } > } > > -/* Callback for qsort that sorts symbols by priority. */ > +/* Callback for std::sort that sorts symbols by priority. */ > > -static int > -compare_msyms (const void *a, const void *b) > +static bool > +compare_msyms (const bound_minimal_symbol &a, const bound_minimal_symbol &b) > { > - const bound_minimal_symbol_d *moa = (const bound_minimal_symbol_d *) a; > - const bound_minimal_symbol_d *mob = (const bound_minimal_symbol_d *) b; > - enum minimal_symbol_type ta = MSYMBOL_TYPE (moa->minsym); > - enum minimal_symbol_type tb = MSYMBOL_TYPE (mob->minsym); > + enum minimal_symbol_type ta = MSYMBOL_TYPE (a.minsym); > + enum minimal_symbol_type tb = MSYMBOL_TYPE (b.minsym); > > - return classify_mtype (ta) - classify_mtype (tb); > + return classify_mtype (ta) < classify_mtype (tb); > } > > /* Callback for iterate_over_minimal_symbols that adds the symbol to > the result. */ This comment should probably updated. It is not really a callback for iterate_over_minimal_symbols anymore (at least not directly). > > static void > -add_minsym (struct minimal_symbol *minsym, void *d) > +add_minsym (struct minimal_symbol *minsym, struct collect_minsyms &info) ... > diff --git a/gdb/minsyms.c b/gdb/minsyms.c > index 72969b7778..08efa1dc7e 100644 > --- a/gdb/minsyms.c > +++ b/gdb/minsyms.c > @@ -471,11 +471,8 @@ linkage_name_str (const lookup_name_info &lookup_name) > void > iterate_over_minimal_symbols (struct objfile *objf, > const lookup_name_info &lookup_name, > - void (*callback) (struct minimal_symbol *, > - void *), > - void *user_data) > + gdb::function_view callback) This line is too long, you should probably wrap the whole parameter list. > { > - > /* The first pass is over the ordinary hash table. */ > { > const char *name = linkage_name_str (lookup_name); > @@ -490,7 +487,7 @@ iterate_over_minimal_symbols (struct objfile *objf, > iter = iter->hash_next) > { > if (mangled_cmp (MSYMBOL_LINKAGE_NAME (iter), name) == 0) > - (*callback) (iter, user_data); > + callback (iter); > } > } > > @@ -509,7 +506,7 @@ iterate_over_minimal_symbols (struct objfile *objf, > iter != NULL; > iter = iter->demangled_hash_next) > if (name_match (MSYMBOL_SEARCH_NAME (iter), lookup_name, NULL)) > - (*callback) (iter, user_data); > + callback (iter); > } > } > > diff --git a/gdb/minsyms.h b/gdb/minsyms.h > index 11a202025d..b05f717575 100644 > --- a/gdb/minsyms.h > +++ b/gdb/minsyms.h > @@ -268,11 +268,9 @@ struct bound_minimal_symbol lookup_minimal_symbol_by_pc (CORE_ADDR); > For each matching symbol, CALLBACK is called with the symbol and > USER_DATA as arguments. */ This comment should be updated. > > -void iterate_over_minimal_symbols (struct objfile *objf, > - const lookup_name_info &name, > - void (*callback) (struct minimal_symbol *, > - void *), > - void *user_data); > +void iterate_over_minimal_symbols > + (struct objfile *objf, const lookup_name_info &name, > + gdb::function_view callback); > > /* Compute the upper bound of MINSYM. The upper bound is the last > address thought to be part of the symbol. If the symbol has a > Thanks, Simon