[PATCH 2/4] remote: C++ify thread_item and threads_listing_context
Simon Marchi
simon.marchi@polymtl.ca
Thu Nov 23 16:48:00 GMT 2017
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
More information about the Gdb-patches
mailing list