This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context
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