[PATCH] D27995: Add the ability for DWARFDie objects to get the parent DWARFDie.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:984-992
+  enum class Tag: uint16_t  {
+    A = dwarf::DW_TAG_lo_user,
+    B,
+    B1,
+    B2,
+    C,
+    C1,
----------------
Is there a need for nodes in this tree to be identifiable by their tags? Could they just be identifiable by pointers or other handles? (eg: could the first test be something like... oh, right, this is roundtripping through DWARF bytes so it's not the same objects on both sides)

Does this test need to roundtrip through DWARF bytes? Could we build some DWARF data types and demonstrate that their parent/sibling walks work correctly after we build them, without having to use DWARF bytes at all? Or is the API not practical to represent in-memory directly without backing bytes?

More like that test at the end that creates DWARFDie and tests it directly, without going through any DWARF byte emission/parsing.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1053-1103
+  // Verify who has children
+  EXPECT_FALSE(A.hasChildren());
+  EXPECT_TRUE(B.hasChildren());
+  EXPECT_TRUE(C.hasChildren());
+
+  // Make sure the parent of all the children of the compile unit are the
+  // compile unit.
----------------
This testing still seems unnecessarily exhaustive - it'd be good to understand which things are being tested and why. Selectively testing interesting parts of the tree walk to ensure things work, without having lots of redundancy - this way tests are easier to maintain and clearer what they are and aren't testing.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1105-1110
+  // Make sure a default constructed DWARFDie doesn't have any parent, sibling
+  // or child;
+  DWARFDie DefaultDie;
+  EXPECT_FALSE(DefaultDie.getParent().isValid());
+  EXPECT_FALSE(DefaultDie.getFirstChild().isValid());
+  EXPECT_FALSE(DefaultDie.getSibling().isValid());
----------------
This looks like it could/should be a separate test case/function - since it's not related to the rest of this function.


https://reviews.llvm.org/D27995