From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58078 invoked by alias); 23 Nov 2017 14:22:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 58063 invoked by uid 89); 23 Nov 2017 14:22:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.7 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Nov 2017 14:22:51 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 223D07EA8B; Thu, 23 Nov 2017 14:22:50 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4641D60F89; Thu, 23 Nov 2017 14:22:49 +0000 (UTC) Subject: Re: [PATCH 2/4] remote: C++ify thread_item and threads_listing_context To: Simon Marchi , gdb-patches@sourceware.org References: <1511368867-19365-1-git-send-email-simon.marchi@ericsson.com> <1511368867-19365-3-git-send-email-simon.marchi@ericsson.com> Cc: Simon Marchi From: Pedro Alves Message-ID: <59086522-06e7-c9a5-a2b3-799d4bc7fb59@redhat.com> Date: Thu, 23 Nov 2017 14:22:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1511368867-19365-3-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-11/txt/msg00538.txt.bz2 On 11/22/2017 04:41 PM, Simon Marchi wrote: > From: Simon Marchi > > 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