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

[Simon Marchi <simark at simark dot ca>]

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