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

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> 
> > But given the nice idea above of defining a "next iterator as end" for
> > another. Would adding that functionality just be introducing a new
> > constructor that takes a Dwarf_Die and a next_sibling () method that
> > returns an iterator that one can use as above in a for loop to test
> > whether one has walked a subtree?
> 
> Yes, that's how I'd do it.
> 
> > The only disadvantage of that seems to be that it would immediately
> > introduce a v2 variant if we do it after a public release.
> 
> I don't even think it would.  Adding a new constructor doesn't break any
> existing clients.  API-wise it would be stable as well, this adds use
> cases, doesn't change any (not even by way of implicit conversions from,
> say, Dwarf_Die * to unit_iterator).

That is a nice surprise. Looking for more background on C++ ABI
compatibility issues I found the following overview handy:
https://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++

So you can always add new functions, including constructors, to classes
and don't break ABI. But because our classes are now not pimpl'd we have
to be careful not to change/add/delete any state data member field or we
will break ABI (and will have to introduce a new v2 namespace).

> 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.

> > I was thinking just doing it on the C++ level. But yes, it would be nice
> > to also have that on the C level libdw interface. That was this
> > discussion where you came up with a pretty nice plan:
> > https://lists.fedorahosted.org/pipermail/elfutils-devel/2014-October/004204.html
> >
> > Again my worry is about whether retrofitting such a design into the
> > current class is possible or not. Would it be as simple as adding a new
> > constructor, a boolean field and using the new C functions (and updating
> > the version) or would we need a whole new class/interface for it?
> 
> 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)?

>   Personally I think adding a new iterator
> that builds on the raw one is the best option.
> 
> That would mean that you couldn't pass a logical iterator to a function
> that expects a raw one.  But you can't likewise pass, say, a child
> iterator to such function--the genericity is just not there.  This sort
> of thing would have to be achieved by making the function template.

The important thing seems to be to be able to turn the iterator into an
actual Dwarf_Die. Which is possible by simply dereferencing the child of
die_tree iterator. So I agree that it isn't too important to be able to
mix-and-match different kinds of iterators.

> One could also make the new iterator configurable, like Frank says in
> his mail.  Then you would have a single tree iterator type that can
> behave either way.

Somehow this appeals more to me. But I admit to not know why. Less
classes feels better somehow. Or maybe it is just that the name
die_tree_iterator sounds more generic than it actually is. Those might
be totally misguided feelings though.

> 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" :)

Thanks,

Mark