This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 7/9] Change solib-aix.c to use type-safe registry
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 <tromey@adacore.com>
>
> * solib-aix.c (library_list_type): New typedef.
> (lm_info_aix_p): Remove typedef. Don't define VEC.
> (struct solib_aix_inferior_data) <library_list>: 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<std::vector<lm_info_aix>> 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<lm_info_aix> library_list_type;
and using gdb::optional<library_list_type> in the code. Or even
gdb::optional<std::vector<lm_info_aix>> is not too long.
> @@ -137,30 +125,30 @@ library_list_start_library (struct gdb_xml_parser *parser,
> void *user_data,
> std::vector<gdb_xml_value> &attributes)
> {
> - VEC (lm_info_aix_p) **list = (VEC (lm_info_aix_p) **) user_data;
> - lm_info_aix *item = new lm_info_aix;
> + std::vector<lm_info_aix> *list = (std::vector<lm_info_aix> *) 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<lm_info_aix> 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