Re: [PATCH 2/2] Add C++ iterators

[Mark Wielaard <mjw at redhat dot com>]

Hi Petr,

On Fri, 2015-04-17 at 18:54 +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> > On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote:
> >> Mark Wielaard <mjw@redhat.com> writes:
> >> As long as your iteration is limited to well-formed sub-tree (i.e. you
> >> don't wan't to e.g. start at one leaf node and progress through half the
> >> CU to another leaf node), the simple vector of offsets would, I think,
> >> be still enough to keep all the context that you need.  There might be
> >> code that assumes that iteration starts and ends at CU DIE's, but that
> >> could be transparently fixed.
> >
> > So, this does concern me a little. We have to be absolutely sure that
> > keeping the state as a vector of offsets is all we ever need.
> 
> The only problem that I see is what to do with the m_cuit.  For
> DIE-to-DIE iteration, it would probably be set to unit_iterator::end().
> That now represents an end-iterator.  So end iterator would be
> represented as m_die.addr == NULL instead--no valid DIE will have a NULL
> address.

OK. I keep mixing issues. But indeed subtree DIE iteration is a separate
from issue the logical/raw walks.

> >> I'm not sure.  I don't think so, at least you would need a different way
> >> of keeping track of context.
> >
> > Could we maybe abstract away the m_stack context (maybe pimpl it)?
> 
> Yes, if you decide to overload the iterator to do both logical and raw
> iteration.  I don't think that's the right approach.

OK.

> I meant the new iterator.  Splitting the concerns into a raw iterator
> and a high-level one that builds upon the raw one absolutely seems the
> right approach to me.  The raw iterator doesn't end up paying the
> overhead for raw iteration, and the complexities of both raw and logical
> iteration will be kept to their respective silos.
> 
> Just to make sure I'm not talking out of my ass, I put together a
> logical_die_tree_iterator.  It's on pmachata/iterators.

Thanks. I'll work on the plain C implementation of "logical" DIE walks,
then we can use that in the future.

> >> I still think exposing raw iterator is the right way forward, because it
> >> is a logical building block.  A logical tree iterator can be built on
> >> top of that even without C support, if that's what's deemed best.
> >
> > If none of the above convinced you otherwise then lets just go with the
> > code you wrote now. But only on the condition that you come up with a
> > non-stupid name for the future "logical_die_tree_iterator" :)
> 
> Note that the raw/logical distinction applies at least to child_iterator
> as well

Yes. If a DIE has a DW_TAG_imported_unit as child, the logical thing to
do is to iterate over its children too.

> , maybe also to unit_iterator.

When using a unit_iterator you should be able to iterate over
(combinations) of the different unit types. But I don't think there is a
logical/raw distinction.

And that is also the only thing I am still slightly confused about.
Because the die_tree_iterator takes a Dwarf or unit_iterator to iterate
over all the DIEs in the Dwarf or (remaining) units, I would like to
indicate which units are interesting (e.g. I only want to iterate over
DIEs in the compile_units and type_units, but not the partial_units).
Should this depend on the type (raw or logical) of the
die_tree_iterator?

>   I think something can be done to
> make logical_die_tree_iterator::move configurable, so that it's useful
> in child iterator as well.
> 
> (One could almost write a template for logical iteration that is
> parametrized by iterator type, but not quite: for the tree iterator one
> needs to skip the root that it's constructed from, but with child
> iterator that's ignored implicitly.  Besides, templates are ABI
> nightmare.)

If we end up with a template implementation, I think it would be better
to just add a boolean flag to the constructors instead to indicate
raw/logical walks.

> Regarding the names, I don't have any good ideas.  In dwgrep I ended up
> with raw and cooked, which is in hindsight perhaps too cute.  Maybe we
> can rename the current bunch to raw_*, and add the logical ones without
> a prefix.

I think that would be best.

Cheers,

Mark