On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote:
Mark Wielaard <mjw(a)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/0042...
>
> 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