From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9387 invoked by alias); 7 Aug 2017 11:46:50 -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 9039 invoked by uid 89); 7 Aug 2017 11:46:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=tripped, Hx-languages-length:2817 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 07 Aug 2017 11:46:42 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v77BkZqr020558 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 7 Aug 2017 07:46:40 -0400 Received: by simark.ca (Postfix, from userid 112) id B2B311EA1C; Mon, 7 Aug 2017 07:46:35 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id DCCFC1E9AB; Mon, 7 Aug 2017 07:46:24 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 07 Aug 2017 11:46:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA] C++-ify skip.c In-Reply-To: <20170806194231.26627-1-tom@tromey.com> References: <20170806194231.26627-1-tom@tromey.com> Message-ID: <5e21aff786d07501d546194f8f48e5bb@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 7 Aug 2017 11:46:35 +0000 X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00116.txt.bz2 On 2017-08-06 21:42, Tom Tromey wrote: > I happened to notice that skiplist_entry, in skip.c, contains a > gdb::optional -- but that this object's destructor is > never run. This can result in a memory leak. > > This patch fixes the bug by applying a bit more C++: changing this > code to use new and delete, and std::unique_ptr; and removing cleanups > in the process. > > Built and regression tested on x86-64 Fedora 25. Ah thanks, I tripped on this with me poison-XNEW patch. I didn't have the time to fix and post it yet. > ChangeLog > 2017-08-06 Tom Tromey > > * skip.c (skiplist_entry): New constructor. > (~skiplist_entry): New destructor. > (skiplist_entry::enabled): Now bool. > (make_skip_entry): Return a unique_ptr. Use new. > (free_skiplist_entry, free_skiplist_entry_cleanup) > (make_free_skiplist_entry_cleanup): Remove. > (skip_command, skip_disable_command, add_skiplist_entry): Update. > (skip_delete_command): Update. Use delete. > --- > gdb/ChangeLog | 11 ++++++++ > gdb/skip.c | 89 > +++++++++++++++++++++++------------------------------------ > 2 files changed, 45 insertions(+), 55 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 722fade..12e0d02 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,14 @@ > +2017-08-06 Tom Tromey > + > + * skip.c (skiplist_entry): New constructor. > + (~skiplist_entry): New destructor. > + (skiplist_entry::enabled): Now bool. > + (make_skip_entry): Return a unique_ptr. Use new. > + (free_skiplist_entry, free_skiplist_entry_cleanup) > + (make_free_skiplist_entry_cleanup): Remove. > + (skip_command, skip_disable_command, add_skiplist_entry): Update. > + (skip_delete_command): Update. Use delete. > + > 2017-08-05 Tom Tromey > > * compile/compile-object-load.c (compile_object_load): Use > diff --git a/gdb/skip.c b/gdb/skip.c > index bf44913..f3291f3 100644 > --- a/gdb/skip.c > +++ b/gdb/skip.c > @@ -38,6 +38,24 @@ > > struct skiplist_entry > { > + skiplist_entry (int file_is_glob_, const char *file_, > + int function_is_regexp_, const char *function_) > + : number (-1), > + file_is_glob (file_is_glob_), file_is_glob and function_is_regexp can probably be changed to bools too. > + file (file_ == NULL ? NULL : xstrdup (file_)), > + function_is_regexp (function_is_regexp_), > + function (function_ == NULL ? NULL : xstrdup (function_)), If we are strdup'ing (making a copy) file and function, we might as well store them in std::strings, so we don't need to explicitly xfree them. And the implicitly defined copy constructor/assignment operator will also work (which is not the case now). An empty string is probably enough to mean the field is not used (which is expressed by a NULL value currently). Thanks, Simon