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

[Petr Machata <pmachata at redhat dot com>]

Mark Wielaard <mjw@redhat.com> writes:

> On Thu, 2015-04-16 at 17:42 +0200, Petr Machata wrote:
>> Mark Wielaard <mjw@redhat.com> writes:
>> > 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++

Yes, that's a great resource.

> 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

Non-virtual member function, including constructors, are essentially the
good old C functions with funny names.  Things break around virtuals.

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

Yes.

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

The only problem that I see is what to do with the m_cuit.  For
DIE-to-DIE iteration, it would probably be set to unit_iterator::end().
That now represents an end-iterator.  So end iterator would be
represented as m_die.addr == NULL instead--no valid DIE will have a NULL
address.

>> 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)?

Yes, if you decide to overload the iterator to do both logical and raw
iteration.  I don't think that's the right approach.

>> Personally I think adding a new iterator that builds on the raw one
>> is the best option.
>
>> 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 meant the new iterator.  Splitting the concerns into a raw iterator
and a high-level one that builds upon the raw one absolutely seems the
right approach to me.  The raw iterator doesn't end up paying the
overhead for raw iteration, and the complexities of both raw and logical
iteration will be kept to their respective silos.

Just to make sure I'm not talking out of my ass, I put together a
logical_die_tree_iterator.  It's on pmachata/iterators.

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

Note that the raw/logical distinction applies at least to child_iterator
as well, maybe also to unit_iterator.  I think something can be done to
make logical_die_tree_iterator::move configurable, so that it's useful
in child iterator as well.

(One could almost write a template for logical iteration that is
parametrized by iterator type, but not quite: for the tree iterator one
needs to skip the root that it's constructed from, but with child
iterator that's ignored implicitly.  Besides, templates are ABI
nightmare.)

Regarding the names, I don't have any good ideas.  In dwgrep I ended up
with raw and cooked, which is in hindsight perhaps too cute.  Maybe we
can rename the current bunch to raw_*, and add the logical ones without
a prefix.

Thanks,
Petr