On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote: > Mark Wielaard <[email protected]> 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
