This is the mail archive of the
mailing list for the elfutils project.
Re: [PATCH 2/2] Add C++ iterators
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 16 Apr 2015 21:23:14 +0200
- Subject: Re: [PATCH 2/2] Add C++ iterators
On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote:
> Mark Wielaard <email@example.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:
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" :)