Re: [PATCH 0/6] change range adapters to be member functions

[Pedro Alves <palves at redhat dot 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<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.  */