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

[Mark Wielaard <mjw@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

On Thu, 2015-04-16 at 15:52 +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> 
> > Is that a common use case? I would have imagined that you would iterate
> > over just the Dwarf_Dies of one particular unit separately or given a
> 
> That would be along these lines:
> 
>   for (die_tree_iterator it (uit), end (++uit); it != end; ++it)
>     ;

O, that is pretty nice. I hadn't thought of that.

> > particular Dwarf_Die would like it to iterate over just the die_tree
> > under that DIE. Given the current design of the die_tree_iterator how
> > would one do that?
> 
> This is not implemented, though I admit it would be a useful feature.
> At this point however I would very much like to keep the scope of the
> patch as it is now.

My problem is that I am not yet so fluent in my C++ thinking to know
whether that would fit the current design or that it would need a
completely different class and implementation. I would very much like to
avoid adding code that we then have to maintain but that we cannot
easily extend to similar use cases and end up with lots of interfaces
and implementations.

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?

The only disadvantage of that seems to be that it would immediately
introduce a v2 variant if we do it after a public release.

> > Also I think that we really should provide an easy way to walk the
> > logical DIE tree. Which means resolving DW_TAG_imported_unit DIEs and
> > just continue with the children of the imported unit at that point.
> > There should of course also be an option to walk the "raw" DIE tree. But
> > it feels wrong to let the user do logical walks on their own, checking
> > for imported_unit tags, walking the imported_unit children tree, keep
> > their own parent stack, etc.
> 
> I agree, this would be tremendously useful--we discussed this recently
> on this list after all.  But it should be implemented in C libdw, and
> only then (optionally) reused in the C++ wrappers.

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?

So my only worry about just adding this code as is now, is that I don't
have a good feeling for how to evolve this C++ code without creating an
unmaintainable mess or abandoning the current code/design. If you could
handwave a plausible path forward to add these things then I would agree
with adding the code as is.

Thanks,

Mark