Re: [PATCH 00/12] Remove some ALL_* iteration macros

[Pedro Alves <palves at redhat dot com>]

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