[PATCH] D28303: Add iterator support to DWARFDie to allow child DIE iteration.

[David Blaikie via llvm-commits]

On Thu, Jan 5, 2017 at 12:38 PM Greg Clayton <clayborg at gmail.com> wrote:

> On Jan 5, 2017, at 11:33 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Looks like it might be missing test coverage for the "skip null" in the
> iterator ctor.
>
>
> I have added a test for this that I’ll post in the next diff.
>
> Also - I'd like a good sense of what's going to happen with the child
> iteration inconsistency between this and the getChild/Sibling API before
> this is committed.
>
>
> DWARFDie::getSibling() will still return a NULL DIE at the end. Iteration
> won’t. What are you suggesting happen?
>
> We could add a bool to the getSibling() API:
>
> DWARFDie DWARFDie::getSibling(bool IncludeNull = false);
>
> Then we could get rid of the code in DWARFDie::iterator and have it
> call DWARFDie::getSibling() with false. The DWARFDie::dump would call it
> with true.
>
> Am I missing something? Let me know what you were thinking.
>

Mostly I was hoping/thinking/suggesting removing the null DIEs as you'd
suggested previously.

It's only a couple of places - one in obj2yaml and one in dwarfdump, yes?
It doesn't seem worth complicating the libDebugInfo APIs for those two use
cases, I think?

Adrian - any thoughts/preferences here?

- Dave


>
> Greg
>
>
> On Thu, Jan 5, 2017 at 11:31 AM Greg Clayton <clayborg at gmail.com> wrote:
>
> On Jan 5, 2017, at 10:53 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Thu, Jan 5, 2017 at 10:46 AM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> clayborg marked an inline comment as done.
> clayborg added inline comments.
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:391
> +    if (Die.isNULL())
> +      Die = DWARFDie();
> +    return *this;
> ----------------
> I would rather not tackle reconstituting the NULL Die in this patch.
>
>
> Sure - sorry, didn't mean to suggest doing it in this patch - but
> potentially frontloading the NULL DIE removal, then committing this patch,
> rather than creating an inconsistent state.
>
>
> I would rather do that if we remove the NULL dies. I think it is fair to
> say that when iterating across the children that we skip the NULL DIEs. I
> can fix the issue where we have only a NULL DIE as the only child.
>
>
> Sure - worst case, could commit this with that fixed & tested, but would
> want a plan/decision on that inconsistency before committing this so we
> don't get left in that weird state.
>
>
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:397
> +  bool operator==(const iterator &X) const { return Die == X.Die; }
> +  const DWARFDie *operator->() const { return &Die; }
> +};
> ----------------
> dblaikie wrote:
> > If you have op* return a reference (as it's meant to to conform to the
> iterator concept in the C++ standard) do you still need to write op->, or
> do you get that for free from the facade?
> If I return a "const DWARFDie &" from operator* I get:
>
> ```
> /Volumes/Data/Users/gclayton/Documents/src/llvm/tot/llvm/include/llvm/ADT/iterator.h:138:12:
> error: cannot initialize return object of type 'llvm::DWARFDie *' with an
> rvalue of type 'const llvm::DWARFDie *'
> ```
>
> For the following code in iterator.h:
>
> ```
>   PointerT operator->() const {
>     return &static_cast<const DerivedT *>(this)->operator*();
>   }
> ```
>
> If I make operator* return a "DWARFDie &" then it works and I can remove
> operator->:
>
> ```
>   DWARFDie &operator*() const { return const_cast<DWARFDie &>(Die); }
> ```
>
> Without the cast it complains about a const function converting to a non
> const DWARFDie &. Which solution do you prefer?
>
>
> I think this might be addressable by passing "const DWARFDie" as the last
> type parameter to the facade helper. Hopefully.
>
>
> Yep, that did the trick. See latest diff which should be good to go.
>
>
>
> ================
> Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:410-412
> +inline iterator_range<DWARFDie::iterator> DWARFDie::children() const {
> +  return make_range(begin(), end());
> +}
> ----------------
> dblaikie wrote:
> > Probably just put this one directly inline in the class, since it can be
> It can't it needs to know what a DWARFDie::iterator is. I tried but got a
> compile error.
>
>
> Ah, right - thanks for the explanation, sorry I didn't spot/understand
> that.
>
>
>
>
> https://reviews.llvm.org/D28303
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170105/fd9ad1b6/attachment.html>