<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jan 5, 2017 at 10:46 AM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg marked an inline comment as done.<br class="gmail_msg">
clayborg added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:391<br class="gmail_msg">
+    if (Die.isNULL())<br class="gmail_msg">
+      Die = DWARFDie();<br class="gmail_msg">
+    return *this;<br class="gmail_msg">
----------------<br class="gmail_msg">
I would rather not tackle reconstituting the NULL Die in this patch. </blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br class="gmail_msg"></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:397<br class="gmail_msg">
+  bool operator==(const iterator &X) const { return Die == X.Die; }<br class="gmail_msg">
+  const DWARFDie *operator->() const { return &Die; }<br class="gmail_msg">
+};<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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?<br class="gmail_msg">
If I return a "const DWARFDie &" from operator* I get:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
/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 *'<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
For the following code in iterator.h:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
  PointerT operator->() const {<br class="gmail_msg">
    return &static_cast<const DerivedT *>(this)->operator*();<br class="gmail_msg">
  }<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
If I make operator* return a "DWARFDie &" then it works and I can remove operator->:<br class="gmail_msg">
<br class="gmail_msg">
```<br class="gmail_msg">
  DWARFDie &operator*() const { return const_cast<DWARFDie &>(Die); }<br class="gmail_msg">
```<br class="gmail_msg">
<br class="gmail_msg">
Without the cast it complains about a const function converting to a non const DWARFDie &. Which solution do you prefer?<br class="gmail_msg"></blockquote><div><br>I think this might be addressable by passing "const DWARFDie" as the last type parameter to the facade helper. Hopefully.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:410-412<br class="gmail_msg">
+inline iterator_range<DWARFDie::iterator> DWARFDie::children() const {<br class="gmail_msg">
+  return make_range(begin(), end());<br class="gmail_msg">
+}<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> Probably just put this one directly inline in the class, since it can be<br class="gmail_msg">
It can't it needs to know what a DWARFDie::iterator is. I tried but got a compile error.<br class="gmail_msg"></blockquote><div><br>Ah, right - thanks for the explanation, sorry I didn't spot/understand that.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D28303" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D28303</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>