From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16896 invoked by alias); 9 Aug 2017 09:15:52 -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 109526 invoked by uid 89); 9 Aug 2017 09:15:23 -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= 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; Wed, 09 Aug 2017 09:15:03 +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 v799EuNS002372 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 9 Aug 2017 05:15:01 -0400 Received: by simark.ca (Postfix, from userid 112) id B35F11EA1D; Wed, 9 Aug 2017 05:14:56 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 116F71E043; Wed, 9 Aug 2017 05:14:42 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 09 Aug 2017 09:15:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA] C++-ify skip.c In-Reply-To: <87zib9hmfa.fsf@tromey.com> References: <20170806194231.26627-1-tom@tromey.com> <5e21aff786d07501d546194f8f48e5bb@polymtl.ca> <87d187cwf3.fsf@pokyo> <19ea4d685104d39dfc20486425f41f37@polymtl.ca> <87zib9hmfa.fsf@tromey.com> Message-ID: <04c97e6e95cf8f012979b6b1052871a0@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 Wed, 9 Aug 2017 09:14:56 +0000 X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00162.txt.bz2 On 2017-08-08 21:47, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi writes: > > Simon> There are many things to fix before the poison-xnew patch can go > in, > Simon> but I think the issue you found is worth fixing sooner than > later. > > Alright, here's a new version. > > Tom > > commit 2d47b197465b640fde68194b3ffe0c5dfa57c134 > Author: Tom Tromey > Date: Sat Aug 5 16:40:56 2017 -0600 > > C++-ify skip.c > > 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. > > ChangeLog > 2017-08-08 Tom Tromey > > * skip.c (skiplist_entry): New constructor. > (skiplist_entry::enabled, > skiplist_entry::function_is_regexp) > (skiplist_entry::file_is_glob): Now bool. > (skiplist_entry::file, skiplist_entry::function): Now > std::string. > (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) > (skip_form_bytes, compile_skip_regexp, skip_command, > skip_info) > (skip_file_p, skip_gfile_p, skip_function_p, > skip_rfunction_p) > (function_name_is_marked_for_skip): Update. > (skip_delete_command): Update. Use delete. > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 722fade..643e407 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,19 @@ > +2017-08-08 Tom Tromey > + > + * skip.c (skiplist_entry): New constructor. > + (skiplist_entry::enabled, skiplist_entry::function_is_regexp) > + (skiplist_entry::file_is_glob): Now bool. > + (skiplist_entry::file, skiplist_entry::function): Now > + std::string. > + (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) > + (skip_form_bytes, compile_skip_regexp, skip_command, skip_info) > + (skip_file_p, skip_gfile_p, skip_function_p, skip_rfunction_p) > + (function_name_is_marked_for_skip): 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..66e9282 100644 > --- a/gdb/skip.c > +++ b/gdb/skip.c > @@ -38,34 +38,44 @@ > > struct skiplist_entry > { > + skiplist_entry (bool file_is_glob_, std::string &&file_, > + bool function_is_regexp_, std::string &&function_) > + : number (-1), > + file_is_glob (file_is_glob_), > + file (file_), > + function_is_regexp (function_is_regexp_), > + function (function_), I think you have to use std::move here to avoid a copy being made. I wasn't sure so I made a small test if you want to try for yourself (or maybe find that I got it wrong): http://paste.ubuntu.com/25275614/ Otherwise, a simpler way would be to leave the constructor (and calling functions) accepting const char* and just change the fields themselves to std::string. The string objects will be constructed using the const char* constructor when the skiplist_entry object itself will be constructed, so you know you for sure you won't have any unnecessary copies. That would avoid complicating the calling functions as well. Either way (adding std::moves or this) are fine for me, it's as you wish. Thanks, Simon