From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23563 invoked by alias); 10 Jul 2019 18:55:13 -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 23404 invoked by uid 89); 10 Jul 2019 18:55:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 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; Wed, 10 Jul 2019 18:55:10 +0000 Received: from [172.16.0.120] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 16DD61E472; Wed, 10 Jul 2019 14:55:09 -0400 (EDT) Subject: Re: [PATCH 7/9] Change solib-aix.c to use type-safe registry To: Tom Tromey , gdb-patches@sourceware.org References: <20190710153947.25721-1-tromey@adacore.com> <20190710153947.25721-8-tromey@adacore.com> From: Simon Marchi Message-ID: Date: Wed, 10 Jul 2019 18:55:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190710153947.25721-8-tromey@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-07/txt/msg00278.txt.bz2 On 2019-07-10 11:39 a.m., Tom Tromey wrote: > This changes solib-aix.c to use the type-safe registry, and removes a > use of VEC in the process. > > gdb/ChangeLog > 2019-07-10 Tom Tromey > > * solib-aix.c (library_list_type): New typedef. > (lm_info_aix_p): Remove typedef. Don't define VEC. > (struct solib_aix_inferior_data) : Change type. > (solib_aix_inferior_data_handle): Change type. > (get_solib_aix_inferior_data): Update. > (solib_aix_free_library_list): Remove. > (library_list_start_library): Update. > (solib_aix_parse_libraries, solib_aix_get_library_list): Change > return type. > (solib_aix_get_library_list) > (solib_aix_solib_create_inferior_hook, solib_aix_current_sos) > (solib_aix_normal_stop_observer, _initialize_solib_aix): Update. > --- > gdb/ChangeLog | 15 ++++++ > gdb/solib-aix.c | 139 ++++++++++++++++-------------------------------- > 2 files changed, 61 insertions(+), 93 deletions(-) > > diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c > index bf2f30d01cd..8f0a3ca9231 100644 > --- a/gdb/solib-aix.c > +++ b/gdb/solib-aix.c > @@ -59,14 +59,13 @@ struct lm_info_aix : public lm_info_base > ULONGEST data_size = 0; > }; > > -typedef lm_info_aix *lm_info_aix_p; > -DEF_VEC_P(lm_info_aix_p); > +typedef gdb::optional> library_list_type; Hmm, I find that hiding the gdb::optional behind a typedef can be a bit confusing. It's easy to forget that it's an optional if it's not "in your face". I think it would help to be explicit and spell out gdb::optional where needed. If you want to keep the typedef, I would suggest defining it as: typedef std::vector library_list_type; and using gdb::optional in the code. Or even gdb::optional> is not too long. > @@ -137,30 +125,30 @@ library_list_start_library (struct gdb_xml_parser *parser, > void *user_data, > std::vector &attributes) > { > - VEC (lm_info_aix_p) **list = (VEC (lm_info_aix_p) **) user_data; > - lm_info_aix *item = new lm_info_aix; > + std::vector *list = (std::vector *) user_data; > + lm_info_aix item; > struct gdb_xml_value *attr; > > attr = xml_find_attribute (attributes, "name"); > - item->filename = xstrdup ((const char *) attr->value.get ()); > + item.filename = (const char *) attr->value.get (); > > attr = xml_find_attribute (attributes, "member"); > if (attr != NULL) > - item->member_name = xstrdup ((const char *) attr->value.get ()); > + item.member_name = (const char *) attr->value.get (); It seems like that fixes a mem leak, we were xstrdup-ing into an std::string. > @@ -237,26 +206,18 @@ static const struct gdb_xml_element library_list_elements[] = > /* Parse LIBRARY, a string containing the loader info in XML format, > and return an lm_info_aix_p vector. The part about "lm_info_aix_p" should be updated. > > - Return NULL if the parsing failed. */ > + Return false if the parsing failed. */ That comment change (Return false) doesn't look in sync with the code (the function doesn't return a bool). > > -static VEC (lm_info_aix_p) * > +static library_list_type > solib_aix_parse_libraries (const char *library) > { > - VEC (lm_info_aix_p) *result = NULL; > - auto cleanup = make_scope_exit ([&] () > - { > - solib_aix_free_library_list (&result); > - }); > + std::vector result; > > if (gdb_xml_parse_quick (_("aix library list"), "library-list-aix.dtd", > - library_list_elements, library, &result) == 0) > - { > - /* Parsed successfully, keep the result. */ > - cleanup.release (); > - return result; > - } > + library_list_elements, library, &result) == 0) > + return library_list_type (std::move (result)); I think you could do "return result;" directly and it will do The Right Thing. > > - return NULL; > + return {}; > } > > #endif /* HAVE_LIBEXPAT */ > @@ -271,14 +232,14 @@ solib_aix_parse_libraries (const char *library) > is not NULL, then print a warning including WARNING_MSG and > a description of the error. */ > > -static VEC (lm_info_aix_p) * > +static library_list_type & > solib_aix_get_library_list (struct inferior *inf, const char *warning_msg) > { The comment above says this function can return NULL, which can't happen if returning a reference. > @@ -530,29 +485,29 @@ static struct so_list * > solib_aix_current_sos (void) > { > struct so_list *start = NULL, *last = NULL; > - VEC (lm_info_aix_p) *library_list; > - lm_info_aix *info; > int ix; > > - library_list = solib_aix_get_library_list (current_inferior (), NULL); > - if (library_list == NULL) > + library_list_type &library_list > + = solib_aix_get_library_list (current_inferior (), NULL); > + if (!library_list.has_value ()) > return NULL; > > /* Build a struct so_list for each entry on the list. > We skip the first entry, since this is the entry corresponding > to the main executable, not a shared library. */ > - for (ix = 1; VEC_iterate (lm_info_aix_p, library_list, ix, info); ix++) > + for (ix = 1; ix < library_list->size (); ix++) This could be: for (lm_info_aix &info : *library_list) Simon