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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-11-23 09:22, Pedro Alves wrote:
> 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 ?

Yes.  As you can see I don't have that reflex yet.  I wish constructors 
were explicit by default though...

> > +
> > +  ~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?

It is changed in the next patch, I thought it would be simpler to change 
it to non-pointer at the same time as the corresponding field in 
remote's private_thread_info.  But now I realize that this state is not 
really good, because the destructor frees the vector and the default 
copy constructor/assignment operator are not deleted, so the vector can 
end up  double-freed.  In any case, I think we'll want to disable copy 
construction and copy assignment (to make sure we don't do copies by 
accident).  If I keep the field as a pointer, then I would need to write 
a custom move constructor that will get removed in the next patch.  So 
instead I'll make the field non-pointer, as you suggest, it ends up 
simpler.  When transferring the thread_handle from the thread_item to 
the private_thread_info, this should be enough:

  info->thread_handle
    = new gdb::byte_vector (std::move (item.thread_handle));

which will get replaced by a simple std::move in the next patch.

> > @@ -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.

I don't think so, here item.name is an std::string, so the assignment 
does a copy.  This copy is required I think, because attr->value won't 
exist after we're done parsing the XML.

> > @@ -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?

Same, extra is an std::string.

Simon