[PATCH] D151001: [DebugInfo][NFCI] Add unittest for DWARFAbbreviationDeclarationSet

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

It would have been better to wait until I'd had a chance to review your latest updates, as there are a few (admittedly minor) points I'd have picked up on that now need you to go and update the in-tree code. I'm also adding a note that I haven't reviewed the coverage of these tests, just the general approach and style.



================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:23
+
+void WriteAbbreviationDeclarations(raw_ostream &OS, uint32_t FirstCode,
+                                   Order Ord) {
----------------
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:58
+  // The Abbreviation Declarations are in order and contiguous, so we want to
+  // make sure that FirstAbbrCode was correctly set
+  EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), FirstCode);
----------------
Comments should be full sentences with correct punctuation (https://llvm.org/docs/CodingStandards.html#commenting).


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:63
+      AbbrevSet.getAbbreviationDeclaration(FirstCode);
+  EXPECT_TRUE(Abbrev5);
+  EXPECT_EQ(Abbrev5->getTag(), DW_TAG_compile_unit);
----------------
If the purpose of this check is to ensure that a valid pointer has come back, this should be `ASSERT_TRUE`, to prevent the test from continuing and crashing if it isn't. Same goes for a few other cases throughout the tests.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:89
+  EXPECT_TRUE(DataWasExtracted);
+  // The declarations are out of order, ensure that FirstAbbrCode is UINT32_MAX
+  EXPECT_EQ(AbbrevSet.getFirstAbbrCode(), UINT32_MAX);
----------------
Same comment as above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151001/new/

https://reviews.llvm.org/D151001