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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 5 11:33:13 PST 2017


Looks like it might be missing test coverage for the "skip null" in the
iterator ctor.

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.

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/b45548a3/attachment.html>


More information about the llvm-commits mailing list