[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:49
+  bool operator !=(const DWARFDie &RHS) const {
+    return Die != RHS.Die || U != RHS.U;
+  }
----------------
Probably best to define != in terms of ==, or the other way around:

  return !(*this == RHS);

(also, I generally suggest that any op overload that can be a non-member should be a non-member: http://stackoverflow.com/questions/4421706/operator-overloading/4421729#4421729 )

& is this clang-formatted? (I'm surprised by the space after the operator keyword)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:382
+public:
+  iterator() : Die() {}
+  explicit iterator(const DWARFDie &D) : Die(D) {}
----------------
  iterator() = default;


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:383
+  iterator() : Die() {}
+  explicit iterator(const DWARFDie &D) : Die(D) {}
+  iterator &operator++() {
----------------
If DWARFDie is cheap enough to copy into a member of an iterator it's probably cheap enough to pass by value here.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:390-391
+  DWARFDie operator*() const { return Die; }
+  bool operator==(const iterator &X) const { return Die == X.Die; }
+  bool operator!=(const iterator &X) const { return Die != X.Die; }
+};
----------------
Does the facade provide a way to only have to implement one of these two?


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:445-447
+llvm::iterator_range<DWARFDie::iterator> DWARFDie::children() const {
+  return make_range(begin(), end());
+}
----------------
This could probably be inline - begin/end wouldn't be bad as inline definitions either.


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1800-1801
+    for (auto Child: DIE.children()) {
+      if (Child.isNULL())
+        break;
       Info.Prune &= analyzeContextInfo(Child, MyIdx, CU, CurrentDeclContext,
----------------
When can a child be null? Oh, we're still leaving null as a child in the iteration. That seems really weird.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1131-1138
+    // CU
+    //   A
+    //   B
+    //     B1
+    //     B2
+    //   C
+    //     C1
----------------
This seems like more testing than I'd expect, given the code change. 

If you want to exercise the same cases for iterator accessors as for the other accessors (could we remove the other accessors (getChild/getSibling) & just rely on iteration instead?) - then perhaps a parameterized test.

Otherwise I'd expect only some fairly brief testing that iterators appear to work - no need to redundantly test the cases that are interesting for getChild/getSibling.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1223-1228
+  // Verify that a NULL DIE has no children.
+  begin = Null.begin();
+  end = Null.end();
+  EXPECT_FALSE((*begin).isValid());
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
----------------
Can you create a NULL DWARFDie object directly & test that separately (in a separate test function)?


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1226-1227
+  end = Null.end();
+  EXPECT_FALSE((*begin).isValid());
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
----------------
can use x->y rather than (*x).y, hopefully


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1230-1236
+  // Verify that an invalid DIE has no children.
+  DWARFDie Invalid;
+  begin = Invalid.begin();
+  end = Invalid.end();
+  EXPECT_FALSE((*begin).isValid());
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
----------------
Split this out as a separate test function


https://reviews.llvm.org/D28303