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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Some remaining ways to simplify the test cases - perhaps Adrian can lend a hand in person. Otherwise I'm happy to go through it in more detail/write up some examples of what I have in mind.



================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1045-1048
+  // Verify all children of the compile unit DIE are correct.
+  EXPECT_EQ(A.getTag(), (dwarf::Tag)Tag::A);
+  EXPECT_EQ(B.getTag(), (dwarf::Tag)Tag::B);
+  EXPECT_EQ(C.getTag(), (dwarf::Tag)Tag::C);
----------------
Not sure if this is interesting to test - I assume we already have pretty good coverage on node tags.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1049-1052
+
+  // Verify who has children
+  EXPECT_FALSE(A.hasChildren());
+  EXPECT_TRUE(B.hasChildren());
----------------
& probably have pretty good coverage on the hasChildren flag too.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1054-1058
+  // Make sure the parent of all the children of the compile unit are the
+  // compile unit.
+  EXPECT_EQ(A.getParent(), CUDie);
+  EXPECT_EQ(B.getParent(), CUDie);
+  EXPECT_EQ(Null.getParent(), CUDie);
----------------
Unless these are interestingly different, I'd just test one.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1060
+
+  EXPECT_FALSE(A.getFirstChild().isValid());
+
----------------
What's the purpose of A? To test a node without children? Perhaps you could use B1 for that instead? (& then probably rename B -> A and C -> B)


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1067-1069
+  // Verify all children of the B DIE correctly valid or invalid.
+  EXPECT_EQ(B1.getTag(), (dwarf::Tag)Tag::B1);
+  EXPECT_EQ(B2.getTag(), (dwarf::Tag)Tag::B2);
----------------
I'd probably skip these too - the tags are sort of interesting, but really it's about the structure we're creating. I don't think checking the tags really makes this test much more robust/provides substantially better coverage for the change being tested.

(which was what motivated my earlier/original suggestion to use some obvious (& pre-existing) tag for everything, to demonstrate that the tags aren't interesting to the test)


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1071-1073
+  // Verify we get invalid children.
+  EXPECT_FALSE(B1.getFirstChild().isValid());
+  EXPECT_FALSE(B2.getFirstChild().isValid());
----------------
Test just one of these? "// Verify that a node without children has an invalid first child" or something like that.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1075-1077
+  // Make sure the parent of all the children of the B are the B.
+  EXPECT_EQ(B1.getParent(), B);
+  EXPECT_EQ(B2.getParent(), B);
----------------
I'd probably skip these - you already tested parental links on A.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1079-1084
+  // 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());
----------------
Pull this out into a separate test function/case.


https://reviews.llvm.org/D27995