<div dir="ltr">Looks like it might be missing test coverage for the "skip null" in the iterator ctor.<br><br>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.</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Jan 5, 2017 at 11:31 AM Greg Clayton <<a href="mailto:clayborg@gmail.com">clayborg@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg">On Jan 5, 2017, at 10:53 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_7552923194316676471Apple-interchange-newline gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Menlo-Regular;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Jan 5, 2017 at 10:46 AM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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.<span class="m_7552923194316676471Apple-converted-space gmail_msg"> </span></blockquote><div class="gmail_msg"><br class="gmail_msg">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 class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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 class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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"> <span class="m_7552923194316676471Apple-converted-space gmail_msg"> </span>PointerT operator->() const {<br class="gmail_msg">   <span class="m_7552923194316676471Apple-converted-space gmail_msg"> </span>return &static_cast<const DerivedT *>(this)->operator*();<br class="gmail_msg"> <span class="m_7552923194316676471Apple-converted-space gmail_msg"> </span>}<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"> <span class="m_7552923194316676471Apple-converted-space gmail_msg"> </span>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 class="gmail_msg"><br class="gmail_msg">I think this might be addressable by passing "const DWARFDie" as the last type parameter to the facade helper. Hopefully.<br class="gmail_msg"></div></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg">Yep, that did the trick. See latest diff which should be good to go.</div></div><div style="word-wrap:break-word" class="gmail_msg"><div class="gmail_msg"><br class="gmail_msg"><blockquote type="cite" class="gmail_msg"><div class="gmail_msg"><div dir="ltr" style="font-family:Menlo-Regular;font-size:12px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="gmail_msg"><div class="gmail_quote gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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 class="gmail_msg"><br class="gmail_msg">Ah, right - thanks for the explanation, sorry I didn't spot/understand that.<br class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></blockquote></div></div></div></blockquote></div><br class="gmail_msg"></div></blockquote></div>