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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:389-391
+    // Don't include the NULL die when iterating.
+    if (Die.isNULL())
+      Die = DWARFDie();
----------------
I think this inconsistency is going to be confusing to users (I suspect the right thing is to only provide the iterator and don't provide the sibling API to users at all - and have the dumpers reconstitute the NULL dies for their own needs)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:389-391
+    // Don't include the NULL die when iterating.
+    if (Die.isNULL())
+      Die = DWARFDie();
----------------
dblaikie wrote:
> I think this inconsistency is going to be confusing to users (I suspect the right thing is to only provide the iterator and don't provide the sibling API to users at all - and have the dumpers reconstitute the NULL dies for their own needs)
I'm guessing this will have problems with a present yet zero-length child list? (in that case the children() iteration would include the null die, since this check isn't done on construction, only on increment)


================
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; }
+};
----------------
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?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:410-412
+inline iterator_range<DWARFDie::iterator> DWARFDie::children() const {
+  return make_range(begin(), end());
+}
----------------
Probably just put this one directly inline in the class, since it can be


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1164-1170
+  // Verify that A has no children by verifying that the begin and end contain
+  // invalid DIEs and also that the iterators are equal.
+  auto begin = A.begin();
+  auto end = A.end();
+  EXPECT_FALSE((*begin).isValid());
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);  
----------------
Testing that they're equal should be sufficient - it's certainly not necessary to define or test what happens when an end iterator is deferenced.


https://reviews.llvm.org/D28303