From patchwork Thu Jan 17 13:58:32 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 31092 Received: (qmail 60977 invoked by alias); 17 Jan 2019 13:58:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 60713 invoked by uid 89); 17 Jan 2019 13:58:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=settled, spread 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, 17 Jan 2019 13:58:34 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B11529D40A; Thu, 17 Jan 2019 13:58:33 +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 DD2E05DD7B; Thu, 17 Jan 2019 13:58:32 +0000 (UTC) Subject: Re: [PATCH 0/6] change range adapters to be member functions To: Tom Tromey , gdb-patches@sourceware.org References: <20190117025032.9265-1-tom@tromey.com> From: Pedro Alves Message-ID: Date: Thu, 17 Jan 2019 13:58:32 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190117025032.9265-1-tom@tromey.com> On 01/17/2019 02:50 AM, Tom Tromey wrote: > In the ALL_* removal series, Pedro pointed out that the range adapter > classes would be nicer as member functions of the relevant class. > > This series implements this idea. Most of these patches were written > using a script. > > While looking at changing the minimal symbol iterator, I noticed that > the implementation could be simplified, so I've included this as patch > #5. > > Regression tested by the buildbot. Thanks. On patches #1 and #2, I'd rather drop the "all_" from the method names, to follow the pattern in the inferiors/threads ranges, where we have: all_threads () all_inferiors () vs inf->threads() When I was doing that initially, I went back and forth picking those names. At some point I had the global functions called just threads() and inferiors(), and that turned out confusing. I also tried naming the inferior method all_threads(), and found that confusing as well. In the end I settled on "all_" for the top level functions because those really iterate over all objects. I.e., all_threads iterates over all threads of all inferiors. While inf->threads() iterates over threads of only that inferior, of course. I tried doing that change locally, and it didn't turn up anything tricky to fix. All we need to do is rename the current program_space::objfiles field, and the only fallback is the object_files macro. See below. I'm not including the rest of the patch since that's just a global search&replace, and you have that spread between two commits, so probably wouldn't help. On patch #5 the commit log says: "array_view is nearly usable, except that this iterator must return pointers rather than references." I found that "must" intriguing. It's not that it must -- with array_view, we could still write code like this: - for (minimal_symbol *msymbol : objfile->msymbols ()) + for (minimal_symbol &msymbol : objfile->msymbols ()) { /* Also handle minimal symbols pointing to function descriptors. */ - if ((MSYMBOL_TYPE (msymbol) == mst_text + if ((MSYMBOL_TYPE (&msymbol) == mst_text } Or this: for (minimal_symbol &msym_ref : objfile->msymbols ()) { minimal_symbol *msymbol = &msym_ref; But that's just not convenient, when most code expects minimal_symbol pointers. So I think it'd be fitting to update the commit log to replace the "must" with "more convenient" or something like it. Maybe even add a comment to the code. Otherwise LGTM. Thanks for doing this. diff --git c/gdb/progspace.h w/gdb/progspace.h index dd97a4e2be..9a061a6e5e 100644 --- c/gdb/progspace.h +++ w/gdb/progspace.h @@ -139,31 +139,32 @@ struct program_space program_space (address_space *aspace_); ~program_space (); - typedef next_adapter all_objfiles_range; + typedef next_adapter objfiles_range; /* Return an iterarable object that can be used to iterate over all - objfiles. The basic use is in a foreach, like: + objfiles in the program space. The basic use is in a foreach, + like: - for (objfile *objf : pspace->all_objfiles ()) { ... } */ - all_objfiles_range all_objfiles () + for (objfile *objf : pspace->objfiles ()) { ... } */ + objfiles_range objfiles () { - return all_objfiles_range (objfiles); + return objfiles_range (objfiles_head); } typedef next_adapter>> - all_objfiles_safe_range; + objfiles_safe_range; /* An iterable object that can be used to iterate over all objfiles. The basic use is in a foreach, like: - for (objfile *objf : pspace->all_objfiles_safe ()) { ... } + for (objfile *objf : pspace->objfiles_safe ()) { ... } This variant uses a basic_safe_iterator so that objfiles can be deleted during iteration. */ - all_objfiles_safe_range all_objfiles_safe () + objfiles_safe_range objfiles_safe () { - return all_objfiles_safe_range (objfiles); + return objfiles_safe_range (objfiles_head); } /* Pointer to next in linked list. */ @@ -218,7 +219,7 @@ struct program_space /* All known objfiles are kept in a linked list. This points to the head of this list. */ - struct objfile *objfiles = NULL; + struct objfile *objfiles_head = NULL; /* The set of target sections matching the sections mapped into this program space. Managed by both exec_ops and solib.c. */ @@ -261,7 +262,7 @@ struct address_space /* All known objfiles are kept in a linked list. This points to the root of this list. */ -#define object_files current_program_space->objfiles +#define object_files current_program_space->objfiles_head /* The set of target sections matching the sections mapped into the current program space. */