[0/6] change range adapters to be member functions
Commit Message
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.
Comments
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> On patches #1 and #2, I'd rather drop the "all_" from the method
Pedro> names, to follow the pattern in the inferiors/threads ranges, where
Pedro> we have:
[...]
I made this change.
Pedro> On patch #5 the commit log says:
Pedro> "array_view is nearly usable, except that this iterator
Pedro> must return pointers rather than references."
Pedro> I found that "must" intriguing. It's not that it
Pedro> must -- with array_view, we could still write code like this:
[...]
Pedro> So I think it'd be fitting to update the commit log to replace
Pedro> the "must" with "more convenient" or something like it. Maybe
Pedro> even add a comment to the code.
I reworded the commit message.
I've re-run this through the buildbot, and I'm going to push it soon.
Tom
@@ -139,31 +139,32 @@ struct program_space
program_space (address_space *aspace_);
~program_space ();
- typedef next_adapter<struct objfile> all_objfiles_range;
+ typedef next_adapter<struct objfile> 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<struct objfile,
basic_safe_iterator<next_iterator<objfile>>>
- 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. */