This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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 itscorresponding fields made std::string too. The xstrdup then becomes anstd::move.Other than that there's nothing really special, it's a usual day-to-dayVEC -> 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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |