[PATCH] D49173: [DebugInfo] Make children iterator bidirectional

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h:354
 inline DWARFDie::iterator DWARFDie::end() const {
-  return iterator();
+  return iterator(getLastChild());
 }
----------------
dblaikie wrote:
> Why was this change needed?
For a bidirectional iterator you should be able to start iterating from `end()`, right? (so long you don't dereference it) 


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:595-599
+  // Find the previous DIE whose depth is the same as the Die's depth.
+  for (size_t I = getDIEIndex(Die) - 1; I >= 0; --I) {
+    if (DieArray[I].getDepth() == Depth)
+      return DWARFDie(this, &DieArray[I]);
+  }
----------------
dblaikie wrote:
> Unlike get(Next)Sibling, this loop may need to check for "<= Depth" and then return DWARFDie() if it's the < case.
> 
> When going forwards, there's always a null die sibling to break this loop (well, I guess a null die is returned from getSibling - and then no caller tries to call getSibling on the null die) - because it has the same depth.
> 
> But going backwards there's no null die to break the loop - oh, well, kind of.
> 
> So if you've got this arrangement: Unit{A{B,NULL},C{D,NULL},NULL}
> 
> And at D you "get previous" then you'll return the NULL child of A... which is still a null child, and that works I suppose. But you could've stopped searching once you saw C - because you've stepped out of C's children list by going to a shallower depth than the depth of D who's sibling you were searching for.
> 
> 
> 
> Oh... and it's undefined to dereference a begin iterator anyway, so none of that really matters, I suppose? :)
Indeed. I added the check at some point but looks like it didn't make it into the patch. Even if it doesn't matter for the iterator (I assume you mean dereferencing the end iterator?) the check is cheap so we might as well make the interface behave as you'd expect. 


https://reviews.llvm.org/D49173