Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context

[Pedro Alves <palves@redhat.com>]

On 11/22/2017 04:41 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> This patch C++ifies the thread_item and threads_listing_context
> structures in remote.c.  thread_item::{extra,name} are changed to
> std::string.  As a result, there's a bit of awkwardness in
> remote_update_thread_list, where we have to xstrdup those strings when
> filling the private_thread_info structure.  This is removed in the
> following patch, where private_thread_info is also C++ified and its
> corresponding fields made std::string too.  The xstrdup then becomes an
> std::move.
> 
> Other than that there's nothing really special, it's a usual day-to-day
> VEC -> vector and char* -> std::string change.  It allows removing a
> cleanup in remote_update_thread_list.
> 
> Note that an overload of hex2bin that returns a gdb::byte_vector is
> added, with corresponding selftests.
> 

Awesome.  Just a couple minor comments below.

> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2972,25 +2972,33 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
>  
>  /* A thread found on the remote target.  */
>  
> -typedef struct thread_item
> +struct thread_item
>  {
> +  thread_item (ptid_t ptid_)
> +  : ptid (ptid_)
> +  {}

Could be explicit ?

> +
> +  ~thread_item ()
> +  {
> +    delete thread_handle;
> +  }
> +
>    /* The thread's PTID.  */
>    ptid_t ptid;
>  
>    /* The thread's extra info.  May be NULL.  */
> -  char *extra;
> +  std::string extra;
>  
>    /* The thread's name.  May be NULL.  */
> -  char *name;
> +  std::string name;
>  
>    /* The core the thread was running on.  -1 if not known.  */
> -  int core;
> +  int core = -1;
>  
>    /* The thread handle associated with the thread.  */
> -  gdb::byte_vector *thread_handle;
> +  gdb::byte_vector *thread_handle = NULL;

This could be made a non-pointer now, right?

> @@ -3109,37 +3105,28 @@ start_thread (struct gdb_xml_parser *parser,
>  {

>    attr = xml_find_attribute (attributes, "name");
> -  item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;
> +  if (attr != NULL)
> +    item.name = (const char *) attr->value;

Are you missing the xstrdup here?  I guess this is related to
the awkwardness you mentioned.

> @@ -3150,8 +3137,8 @@ end_thread (struct gdb_xml_parser *parser,
>    struct threads_listing_context *data
>      = (struct threads_listing_context *) user_data;
>  
> -  if (body_text && *body_text)
> -    VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
> +  if (body_text != NULL && *body_text != '\0')
> +    data->items.back ().extra = body_text;

And here?

Thanks,
Pedro Alves