[00/12] Remove some ALL_* iteration macros

Message ID 20181125165439.13773-1-tom@tromey.com
Headers show
Series
  • Remove some ALL_* iteration macros
Related show

Message

Tom Tromey Nov. 25, 2018, 4:54 p.m.
This series removes various ALL_* iteration macros, in favor of C++
iterators, range adapters, and ranged for loops.  I've been wanting
this for a while, because it helps a little bit with various
experiments of mine that involve changing objfile lifetime management;
Pedro's thread iterator patch prompted me to finally do this.

The main downside of removing these macros is that it involves some
reindentation; and expanding some macros to two nested loops means a
couple somewhat ugly reformattings.

On the plus side, though, this tightens the scope of iteration
variables, which is good.  And, it removes some hairy code,
particularly the ALL_OBJSECTIONS patch.

There are still a few more such macros that could be converted.  And,
I think inf_threads_iterator could be converted to use next_iterator.
I can do some of this if there's interest.

Regression tested by the buildbot.

Tom

Comments

Joel Brobecker Dec. 23, 2018, 7 a.m. | #1
> This series removes various ALL_* iteration macros, in favor of C++

> iterators, range adapters, and ranged for loops.  I've been wanting

> this for a while, because it helps a little bit with various

> experiments of mine that involve changing objfile lifetime management;

> Pedro's thread iterator patch prompted me to finally do this.

> 

> The main downside of removing these macros is that it involves some

> reindentation; and expanding some macros to two nested loops means a

> couple somewhat ugly reformattings.

> 

> On the plus side, though, this tightens the scope of iteration

> variables, which is good.  And, it removes some hairy code,

> particularly the ALL_OBJSECTIONS patch.

> 

> There are still a few more such macros that could be converted.  And,

> I think inf_threads_iterator could be converted to use next_iterator.

> I can do some of this if there's interest.

> 

> Regression tested by the buildbot.


FWIW, this seems like a good change to me; I am particularly
receptive to the fact that the scope of the iteration variables
is now restricted to the loop itself.

As for the downside, I don't consider re-indentation a downside
other than it makes review a bit more painful. The two-nested
loop didn't seem like they were making the code less readable.
Did you have any other concerns with that?

I admit I was scanning the last few patches much faster than the first
few ones, but they seem fairly mechanical to me, so I think the risk
is low.

-- 
Joel
Tom Tromey Dec. 24, 2018, 8:54 p.m. | #2
>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:


Joel> As for the downside, I don't consider re-indentation a downside
Joel> other than it makes review a bit more painful. The two-nested
Joel> loop didn't seem like they were making the code less readable.
Joel> Did you have any other concerns with that?

Nope.

Joel> I admit I was scanning the last few patches much faster than the first
Joel> few ones, but they seem fairly mechanical to me, so I think the risk
Joel> is low.

I think I will have to redo much of the series to account for that
version of gcc that Simon sometimes uses -- the one that requires ranged
for loops to use the "struct" keyword.  I probably can't really test
that change but I'll make a best effort at it in order to reduce the
amount of work he has to do.

Tom
Simon Marchi Dec. 26, 2018, 5:30 p.m. | #3
On 2018-12-24 3:54 p.m., Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

> 

> Joel> As for the downside, I don't consider re-indentation a downside

> Joel> other than it makes review a bit more painful. The two-nested

> Joel> loop didn't seem like they were making the code less readable.

> Joel> Did you have any other concerns with that?

> 

> Nope.

> 

> Joel> I admit I was scanning the last few patches much faster than the first

> Joel> few ones, but they seem fairly mechanical to me, so I think the risk

> Joel> is low.

> 

> I think I will have to redo much of the series to account for that

> version of gcc that Simon sometimes uses -- the one that requires ranged

> for loops to use the "struct" keyword.  I probably can't really test

> that change but I'll make a best effort at it in order to reduce the

> amount of work he has to do.


Hehe.  Looking back at those patches, it's with gcc 6.3.0.  Those are cross-compilers
I built with crosstool-ng 1.22:

$ ~/x-tools/aarch64-rpi3-linux-gnueabi/bin/aarch64-rpi3-linux-gnueabi-gcc --version
aarch64-rpi3-linux-gnueabi-gcc (crosstool-NG crosstool-ng-1.22.0-677-ga3dd55b9) 6.3.0

I see crosstool-ng 1.23 has been released, but it still uses gcc 6.3.0.  And it looks like
the gcc version used in current Debian stable (codename stretch) is also 6.3.0:

  https://packages.debian.org/stretch/gcc

I'll give a try to this patchset (and take a quick look at the same time).

Simon
Tom Tromey Dec. 26, 2018, 10:28 p.m. | #4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I'll give a try to this patchset (and take a quick look at the
Simon> same time).

It's on submit/remove-loop-macros on my github.
I rebased it today.

Tom
Tom Tromey Dec. 26, 2018, 10:32 p.m. | #5
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I'll give a try to this patchset (and take a quick look at the
Simon> same time).

Tom> It's on submit/remove-loop-macros on my github.
Tom> I rebased it today.

Apparently I thought ahead and used the struct tags when I wrote the
patch.  Funny that I don't remember that.

I'll push it in some day soon.

Tom
Simon Marchi Dec. 26, 2018, 10:35 p.m. | #6
On 2018-12-26 5:32 p.m., Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

> 

>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> Simon> I'll give a try to this patchset (and take a quick look at the

> Simon> same time).

> 

> Tom> It's on submit/remove-loop-macros on my github.

> Tom> I rebased it today.

> 

> Apparently I thought ahead and used the struct tags when I wrote the

> patch.  Funny that I don't remember that.

> 

> I'll push it in some day soon.

> 

> Tom

> 


Usually, the problem I had was the other way around, that having the struct tag
causes a compilation problem:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=8634679f82df75cf482b0c0814c2b3865da91d22

The recent Hurd problem, however, happened when not using the struct tag
with thread_info...

Simon
Tom Tromey Dec. 26, 2018, 10:52 p.m. | #7
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> Usually, the problem I had was the other way around, that having the struct tag
Simon> causes a compilation problem:

Aha, that's what I misremembered.
Ok, I can fix that.

Tom
Tom Tromey Dec. 26, 2018, 11:45 p.m. | #8
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Usually, the problem I had was the other way around, that having the struct tag
Simon> causes a compilation problem:

Tom> Aha, that's what I misremembered.
Tom> Ok, I can fix that.

I did it.  A few spots needed an additional tweak to avoid a clash
between a variable ("objfile" or "symtab") and the type.
I've force-pushed it to that same branch, in case you feel like giving
it a try.  I'll re-send the series sometime.

Tom
Simon Marchi Dec. 27, 2018, 12:45 a.m. | #9
On 2018-12-26 18:45, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

> 

>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> Simon> Usually, the problem I had was the other way around, that

> having the struct tag

> Simon> causes a compilation problem:

> 

> Tom> Aha, that's what I misremembered.

> Tom> Ok, I can fix that.

> 

> I did it.  A few spots needed an additional tweak to avoid a clash

> between a variable ("objfile" or "symtab") and the type.

> I've force-pushed it to that same branch, in case you feel like giving

> it a try.  I'll re-send the series sometime.


When I played with it, the compiler did not complain when I used:

   for (objfile *objfile : ...)

That was with gcc 8.2.0.  Do you know if it's really problematic with 
other compilers/versions?

Simon
Simon Marchi Dec. 27, 2018, 1:52 a.m. | #10
On 2018-11-25 11:54 a.m., Tom Tromey wrote:
> This series removes various ALL_* iteration macros, in favor of C++

> iterators, range adapters, and ranged for loops.  I've been wanting

> this for a while, because it helps a little bit with various

> experiments of mine that involve changing objfile lifetime management;

> Pedro's thread iterator patch prompted me to finally do this.

> 

> The main downside of removing these macros is that it involves some

> reindentation; and expanding some macros to two nested loops means a

> couple somewhat ugly reformattings.

> 

> On the plus side, though, this tightens the scope of iteration

> variables, which is good.  And, it removes some hairy code,

> particularly the ALL_OBJSECTIONS patch.

> 

> There are still a few more such macros that could be converted.  And,

> I think inf_threads_iterator could be converted to use next_iterator.

> I can do some of this if there's interest.

> 

> Regression tested by the buildbot.

> 

> Tom


I gave that a quick look.

I was wondering if you had thought about replacing, for example

  ALL_COMPUNITS (objfile, s)

with an equivalent like this

  for (compunit_symtab *s : all_compunits (current_program_space))

in order to avoid nested loops like

  for (objfile *objfile : all_objfiles (current_program_space))
    for (compunit_symtab *s : objfile_compunits (objfile))
      {
        ...
        ...
      }

In most cases, the objfile variable is not needed for anything else than
iterating on the compunit_symtabs.  For cases where the outer iteration
variable is needed, then we can use the nested loops.

I am not sure which one I like best.  The flat version reduces indentation, but
the nested version makes it clear and explicit how the data is represented, so
it might help readers who are less familiar with the code.

Also, in theory, according to the coding style, we should write

for (...)
  {
    for (...)
      {
        ...
        ...
      }
  }

Simon
Tom Tromey Dec. 27, 2018, 6:22 a.m. | #11
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> When I played with it, the compiler did not complain when I used:
Simon>   for (objfile *objfile : ...)
Simon> That was with gcc 8.2.0.  Do you know if it's really problematic with
Simon> other compilers/versions?

That case is fine, but the problem is a case like:

for (objfile *objfile : ...)
  for (objfile *objfile2 : ...)

Here the compiler complains about the inner for, because 'objfile' is a
variable.

Another case was one where 'symtab' was the name of a function
parameter, causing problems with "for (symbtab *... : ...)"

Tom
Tom Tromey Jan. 3, 2019, 9:45 p.m. | #12
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I was wondering if you had thought about replacing, for example
Simon>   ALL_COMPUNITS (objfile, s)
Simon> with an equivalent like this
Simon>   for (compunit_symtab *s : all_compunits (current_program_space))
Simon> in order to avoid nested loops like
[...]

Yeah, I don't think I really considered it.

Simon> I am not sure which one I like best.  The flat version reduces indentation, but
Simon> the nested version makes it clear and explicit how the data is represented, so
Simon> it might help readers who are less familiar with the code.

Same for me.  Maybe I lean a bit toward the explicit form but that might
only be because I already have the patch in hand.

Simon> Also, in theory, according to the coding style, we should write
Simon> for (...)
Simon>   {
Simon>     for (...)
Simon>       {
Simon>         ...
Simon>         ...
Simon>       }
Simon>   }

I thought it was ok to leave a single statement unbraced, though I
personally never do this for something like:

   for (...)
     if ...
     else...

...since I think that's less readable than the braced version.

If the braces are needed then that probably argues for a smarter
iterator, to avoid excessive indentation.

Tom
Simon Marchi Jan. 3, 2019, 10:45 p.m. | #13
On 2019-01-03 16:45, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

> Simon> I was wondering if you had thought about replacing, for example

> Simon>   ALL_COMPUNITS (objfile, s)

> Simon> with an equivalent like this

> Simon>   for (compunit_symtab *s : all_compunits 

> (current_program_space))

> Simon> in order to avoid nested loops like

> [...]

> 

> Yeah, I don't think I really considered it.

> 

> Simon> I am not sure which one I like best.  The flat version reduces

> indentation, but

> Simon> the nested version makes it clear and explicit how the data is

> represented, so

> Simon> it might help readers who are less familiar with the code.

> 

> Same for me.  Maybe I lean a bit toward the explicit form but that 

> might

> only be because I already have the patch in hand.

> 

> Simon> Also, in theory, according to the coding style, we should write

> Simon> for (...)

> Simon>   {

> Simon>     for (...)

> Simon>       {

> Simon>         ...

> Simon>         ...

> Simon>       }

> Simon>   }

> 

> I thought it was ok to leave a single statement unbraced, though I

> personally never do this for something like:

> 

>    for (...)

>      if ...

>      else...

> 

> ...since I think that's less readable than the braced version.

> 

> If the braces are needed then that probably argues for a smarter

> iterator, to avoid excessive indentation.


They are needed if we want to strictly follow the GNU coding style:

https://www.gnu.org/prep/standards/standards.html#Clean-Use-of-C-Constructs

I think what you did is easy to read, since it's pretty straightforward. 
  We could always make an exception for these constructs, but it would 
probably end up being confusing to understand and explain when you can 
omit the braces and when you can't.

If we end up using "smarter iterators" (for the lack of a better name) 
they could be overloads:

/* Iterate on all compunits of an objfile.  */
... all_compunits (objfile *);
/ Iterate on all compunits of a program space.  */
... all_compunits (program_space *);

And let's say that all_compunits (program_space *) returns tuples of 
<objfile *, compunit_symtab *>, we'll be able to use structured bindings 
when we switch to C++17 :).  Something like:

for (const auto &[objfile, compunit] : all_compunits (pspace))

Simon
Tom Tromey Jan. 6, 2019, 8:10 p.m. | #14
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> I think what you did is easy to read, since it's pretty
Simon> straightforward. We could always make an exception for these
Simon> constructs, but it would probably end up being confusing to understand
Simon> and explain when you can omit the braces and when you can't.

[...]

Simon> And let's say that all_compunits (program_space *) returns tuples of
Simon> <objfile *, compunit_symtab *>, we'll be able to use structured
Simon> bindings when we switch to C++17 :).  Something like:

Simon> for (const auto &[objfile, compunit] : all_compunits (pspace))

I gave this a brief try.  I wrote a "nested" iterator to make this
generic and then converted all_compunits.

With the nested iterator, one needs to write:

  for (auto blah : all_compunits ())
    {
      compunit_symtab *s = blah.second;

This looked ugly to me.


Alternatively, we'd have to write a "second-selecting" wrapper iterator.
I didn't implement it but I suppose it would look like:

  for (compunit_symtab *s : second_adapter<all_compunits> ())
     ...

This seemed a bit obscure to me, though.

In the end I think I prefer the explicit route.  It does need more
indentation (I will add the missing braces), but the explicitness seems
good.  To me, there doesn't seem to be a strong reason to prefer the
shorter formulations; and the new iterators and adapters that would be
needed are just a bunch more code to try to track through.

Let me know what you think about this.  If you like the second_adapter
approach, I can implement that.

Tom
Simon Marchi Jan. 9, 2019, 7:49 p.m. | #15
On 2019-01-06 15:10, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

> Simon> I think what you did is easy to read, since it's pretty

> Simon> straightforward. We could always make an exception for these

> Simon> constructs, but it would probably end up being confusing to 

> understand

> Simon> and explain when you can omit the braces and when you can't.

> 

> [...]

> 

> Simon> And let's say that all_compunits (program_space *) returns 

> tuples of

> Simon> <objfile *, compunit_symtab *>, we'll be able to use structured

> Simon> bindings when we switch to C++17 :).  Something like:

> 

> Simon> for (const auto &[objfile, compunit] : all_compunits (pspace))

> 

> I gave this a brief try.  I wrote a "nested" iterator to make this

> generic and then converted all_compunits.

> 

> With the nested iterator, one needs to write:

> 

>   for (auto blah : all_compunits ())

>     {

>       compunit_symtab *s = blah.second;

> 

> This looked ugly to me.


In that case it would always return a specialized struct

   struct {
     objfile *objfile;
     compunit_symtab *compunit;
   };

> Alternatively, we'd have to write a "second-selecting" wrapper 

> iterator.

> I didn't implement it but I suppose it would look like:

> 

>   for (compunit_symtab *s : second_adapter<all_compunits> ())

>      ...

> 

> This seemed a bit obscure to me, though.

> 

> In the end I think I prefer the explicit route.  It does need more

> indentation (I will add the missing braces), but the explicitness seems

> good.  To me, there doesn't seem to be a strong reason to prefer the

> shorter formulations; and the new iterators and adapters that would be

> needed are just a bunch more code to try to track through.

> 

> Let me know what you think about this.  If you like the second_adapter

> approach, I can implement that.


Yeah, I think the second_adapter approach just makes things more 
obscure.

Really, I am fine with what you did.

Simon
Tom Tromey Jan. 10, 2019, 1:29 a.m. | #16
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> Yeah, I think the second_adapter approach just makes things more
Simon> obscure.

Simon> Really, I am fine with what you did.

Ok.  I added the missing braces and re-tested it on the buildbot
("Fedora-x86_64-m64"); and I am going to push it now.

Tom
Pedro Alves Jan. 10, 2019, 4:44 p.m. | #17
Hi Tom,

On 11/25/2018 04:54 PM, Tom Tromey wrote:
> This series removes various ALL_* iteration macros, in favor of C++

> iterators, range adapters, and ranged for loops.  I've been wanting

> this for a while, because it helps a little bit with various

> experiments of mine that involve changing objfile lifetime management;

> Pedro's thread iterator patch prompted me to finally do this.


I'm only now catching up with this.  Thanks so much for doing it!

> There are still a few more such macros that could be converted.  And,

> I think inf_threads_iterator could be converted to use next_iterator.

> I can do some of this if there's interest.


If it's a natural fit, then I think it'd be nice.  Is there a downside?

Thanks,
Pedro Alves
Pedro Alves Jan. 10, 2019, 4:45 p.m. | #18
On 01/06/2019 08:10 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

> 

> Simon> I think what you did is easy to read, since it's pretty

> Simon> straightforward. We could always make an exception for these

> Simon> constructs, but it would probably end up being confusing to understand

> Simon> and explain when you can omit the braces and when you can't.

> 

> [...]

> 

> Simon> And let's say that all_compunits (program_space *) returns tuples of

> Simon> <objfile *, compunit_symtab *>, we'll be able to use structured

> Simon> bindings when we switch to C++17 :).  Something like:

> 

> Simon> for (const auto &[objfile, compunit] : all_compunits (pspace))

> 

> I gave this a brief try.  I wrote a "nested" iterator to make this

> generic and then converted all_compunits.

> 

> With the nested iterator, one needs to write:

> 

>   for (auto blah : all_compunits ())

>     {

>       compunit_symtab *s = blah.second;

> 

> This looked ugly to me.


Yeah.  Though, I think that if we took this route, then 

  all_compunits ()

could still return just a compunit pointer, not two things:

  compunit_symtab *all_compunits ()

because you can always get the objfile pointer from compunit_symtab::objfile.

> 

> 

> Alternatively, we'd have to write a "second-selecting" wrapper iterator.

> I didn't implement it but I suppose it would look like:

> 

>   for (compunit_symtab *s : second_adapter<all_compunits> ())

>      ...

> 

> This seemed a bit obscure to me, though.


Definitely.

> 

> In the end I think I prefer the explicit route.  It does need more

> indentation (I will add the missing braces), but the explicitness seems

> good.  To me, there doesn't seem to be a strong reason to prefer the

> shorter formulations; and the new iterators and adapters that would be

> needed are just a bunch more code to try to track through.

> 

> Let me know what you think about this.  If you like the second_adapter

> approach, I can implement that.


Me, I don't.


BTW, in

> +/* A range adapter that makes it possible to iterate over all

> +   compunits in one objfile.  */

> +

> +class objfile_compunits : public next_adapter<struct compunit_symtab>

> +{

> +public:

>


in the threads/inferiors iterators I named the range types xxx_range
and then added methods to return the range instead of calling
the ctor directly.  In this case, it'd be the equivalent of naming
the class above

  class objfile_compunits_range : public next_adapter<struct compunit_symtab>
  {

and then adding a method to objfile::compunits() method like:

  objfile_compunits_range compunits ()
  { return objfile_compunits_range (this); }

Then the client code would look like:

> -	ALL_OBJFILE_COMPUNITS (objfile, cust)

> +	for (struct compunit_symtab *cust : objfile->compunits ())


Instead of:

 > -	ALL_OBJFILE_COMPUNITS (objfile, cust)

 > +	for (struct compunit_symtab *cust : objfile_compunits (objfile))


At the time, I thought that read better on the client side.  I.e.,
we have:

  inf->threads ()

instead of:

  inf_threads (inf)

OOC, did you consider following that, and decided again?
No need to redo the series or anything, I'm just curious,
since I would have taken a different choice.

Thanks,
Pedro Alves
Tom Tromey Jan. 10, 2019, 6:06 p.m. | #19
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


>> There are still a few more such macros that could be converted.  And,

>> I think inf_threads_iterator could be converted to use next_iterator.

>> I can do some of this if there's interest.


Pedro> If it's a natural fit, then I think it'd be nice.  Is there a downside?

I don't think there is one.
What do you think of this?

Tom

commit f4cd9886913b5216ed6073af23d250401f006551
Author: Tom Tromey <tom@tromey.com>
Date:   Thu Jan 10 10:50:43 2019 -0700

    Replace inf_threads_iterator with next_iterator
    
    This changes inf_threads_iterator and some range adapters in
    thread-iter.h to use next_iterator and next_adapter instead.
    
    gdb/ChangeLog
    2019-01-10  Tom Tromey  <tom@tromey.com>
    
            * thread-iter.h (inf_threads_iterator): Use next_iterator.
            (basic_inf_threads_range): Remove.
            (inf_threads_range, inf_non_exited_threads_range)
            (safe_inf_threads_range): Use next_adapter.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 32fe0bbe814..ac1dd540ef7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-01-10  Tom Tromey  <tom@tromey.com>
+
+	* thread-iter.h (inf_threads_iterator): Use next_iterator.
+	(basic_inf_threads_range): Remove.
+	(inf_threads_range, inf_non_exited_threads_range)
+	(safe_inf_threads_range): Use next_adapter.
+
 2019-01-10  Tom Tromey  <tom@tromey.com>
 
 	* objfiles.h (objfile::reset_psymtabs): Update.
diff --git a/gdb/thread-iter.h b/gdb/thread-iter.h
index a1145d4938a..be6ab73c686 100644
--- a/gdb/thread-iter.h
+++ b/gdb/thread-iter.h
@@ -20,68 +20,13 @@
 #define THREAD_ITER_H
 
 #include "common/filtered-iterator.h"
+#include "common/next-iterator.h"
 #include "common/safe-iterator.h"
 
 /* A forward iterator that iterates over a given inferior's
    threads.  */
 
-class inf_threads_iterator
-{
-public:
-  typedef inf_threads_iterator self_type;
-  typedef struct thread_info *value_type;
-  typedef struct thread_info *&reference;
-  typedef struct thread_info **pointer;
-  typedef std::forward_iterator_tag iterator_category;
-  typedef int difference_type;
-
-  /* Create an iterator pointing at HEAD.  This takes a thread pointer
-     instead of an inferior pointer to avoid circular dependencies
-     between the thread and inferior header files.  */
-  explicit inf_threads_iterator (struct thread_info *head)
-    : m_thr (head)
-  {}
-
-  /* Create a one-past-end iterator.  */
-  inf_threads_iterator ()
-    : m_thr (nullptr)
-  {}
-
-  inf_threads_iterator& operator++ ()
-  {
-    m_thr = m_thr->next;
-    return *this;
-  }
-
-  thread_info *operator* () const { return m_thr; }
-
-  bool operator!= (const inf_threads_iterator &other) const
-  { return m_thr != other.m_thr; }
-
-private:
-  /* The currently-iterated thread.  NULL if we reached the end of the
-     list.  */
-  thread_info *m_thr;
-};
-
-/* A range adapter that makes it possible to iterate over an
-   inferior's thread list with range-for.  */
-template<typename Iterator>
-struct basic_inf_threads_range
-{
-  friend struct inferior;
-private:
-  explicit basic_inf_threads_range (struct thread_info *head)
-    : m_head (head)
-  {}
-
-public:
-  Iterator begin () const { return Iterator (m_head); }
-  Iterator end () const { return Iterator (); }
-
-private:
-  thread_info *m_head;
-};
+using inf_threads_iterator = next_iterator<thread_info>;
 
 /* A forward iterator that iterates over all threads of all
    inferiors.  */
@@ -223,19 +168,19 @@ using safe_inf_threads_iterator
    of an inferior with range-for.  */
 
 using inf_threads_range
-  = basic_inf_threads_range<inf_threads_iterator>;
+  = next_adapter<thread_info, inf_threads_iterator>;
 
 /* A range adapter that makes it possible to iterate over all
    non-exited threads of an inferior with range-for.  */
 
 using inf_non_exited_threads_range
-  = basic_inf_threads_range<inf_non_exited_threads_iterator>;
+  = next_adapter<thread_info, inf_non_exited_threads_iterator>;
 
 /* A range adapter that makes it possible to iterate over all threads
    of an inferior with range-for, safely.  */
 
 using safe_inf_threads_range
-  = basic_inf_threads_range<safe_inf_threads_iterator>;
+  = next_adapter<thread_info, safe_inf_threads_iterator>;
 
 /* A range adapter that makes it possible to iterate over all threads
    of all inferiors with range-for.  */
Pedro Alves Jan. 10, 2019, 6:09 p.m. | #20
On 01/10/2019 06:06 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

>>> There are still a few more such macros that could be converted.  And,

>>> I think inf_threads_iterator could be converted to use next_iterator.

>>> I can do some of this if there's interest.

> 

> Pedro> If it's a natural fit, then I think it'd be nice.  Is there a downside?

> 

> I don't think there is one.

> What do you think of this?


Looks good, thanks!

Pedro Alves
Tom Tromey Jan. 10, 2019, 6:10 p.m. | #21
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Yeah.  Though, I think that if we took this route, then 
Pedro>   all_compunits ()
Pedro> could still return just a compunit pointer, not two things:
Pedro>   compunit_symtab *all_compunits ()
Pedro> because you can always get the objfile pointer from compunit_symtab::objfile.

In the long run I still hope to get rid of compunit_symtab::objfile.
Though I suppose this could just be added to the future to-do list.

[...]
Pedro> in the threads/inferiors iterators I named the range types xxx_range
Pedro> and then added methods to return the range instead of calling
Pedro> the ctor directly.  In this case, it'd be the equivalent of naming
Pedro> the class above

Pedro>   class objfile_compunits_range : public next_adapter<struct compunit_symtab>
Pedro>   {

Pedro> and then adding a method to objfile::compunits() method like:

Pedro>   objfile_compunits_range compunits ()
Pedro>   { return objfile_compunits_range (this); }

[...]

Pedro> OOC, did you consider following that, and decided again?
Pedro> No need to redo the series or anything, I'm just curious,
Pedro> since I would have taken a different choice.

I didn't even think of it.  I will add this to my to-do list, it does
look better.

Also maybe I next_adapter should be renamed to something better, at
least having the word "range" in it?

Tom
Pedro Alves Jan. 10, 2019, 7:58 p.m. | #22
On 01/10/2019 06:10 PM, Tom Tromey wrote:

> Pedro> OOC, did you consider following that, and decided again?

> Pedro> No need to redo the series or anything, I'm just curious,

> Pedro> since I would have taken a different choice.

> 

> I didn't even think of it.  I will add this to my to-do list, it does

> look better.

> 

> Also maybe I next_adapter should be renamed to something better, at

> least having the word "range" in it?

> 


Yeah.

Thanks,
Pedro Alves
Tom Tromey Jan. 10, 2019, 10:53 p.m. | #23
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


>> I don't think there is one.

>> What do you think of this?


Pedro> Looks good, thanks!

I'm going to check it in.

Tom