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

[Greg Clayton via llvm-commits]

> 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 <mailto: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 <https://reviews.llvm.org/D28303>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170105/be98bb52/attachment.html>