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

[Simon Marchi <simon.marchi@polymtl.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