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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:447
+  return make_range(begin(), end());
+}
----------------
I can inline children(), but not begin() and end() since they use the iterator class which must be forward declared since it has a member variable:

```
  DWARFDie Die;
```



================
Comment at: tools/dsymutil/DwarfLinker.cpp:1800-1801
+    for (auto Child: DIE.children()) {
+      if (Child.isNULL())
+        break;
       Info.Prune &= analyzeContextInfo(Child, MyIdx, CU, CurrentDeclContext,
----------------
dblaikie wrote:
> When can a child be null? Oh, we're still leaving null as a child in the iteration. That seems really weird.
Yeah, we currently want it there for dumping, but not for real world use. Should we add a "bool IncludeNull" as a parameter to the children function to allow clients to choose?


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1138
+    //     C1
+    //   D
+    auto CUDie = CU.getUnitDIE();
----------------
ok, I can simplify and remove B1, B2, C, C1 and D.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1226-1227
+  end = Null.end();
+  EXPECT_FALSE((*begin).isValid());
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
----------------
dblaikie wrote:
> can use x->y rather than (*x).y, hopefully
I currently return a DWARFDie instance from the operator * for DWARFDie::iterator as I didn't want clients to modify the DWARFDie in the iterator by returning it by reference since it is what controls the iteration. This makes the -> operator not work as it takes the address of the returned DWARFDie and causes a compile error. Should we just have "DWARFDie::iterator operator*" return a reference to the contained item? I didn't like either approach.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1228
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
+
----------------
No, you can't get a NULL die without creating DWARF for it, so this should probably stay here.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1236
+  EXPECT_FALSE((*end).isValid());
+  EXPECT_EQ(begin, end);
+}
----------------
Why? this is testing the iterators and their functionality.


https://reviews.llvm.org/D28303