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

[Greg Clayton via llvm-commits]

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

Greg

> 
> On Thu, Jan 5, 2017 at 11:31 AM Greg Clayton <clayborg at gmail.com <mailto:clayborg at gmail.com>> wrote:
>> On Jan 5, 2017, at 10:53 AM, David Blaikie <dblaikie at gmail.com <mailto: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/ef8c9640/attachment.html>