Re: [PATCH 7/9] Change solib-aix.c to use type-safe registry

[Simon Marchi <simark at simark dot ca>]

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