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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the tests - always useful. I've got a couple of comments/suggestions for you.



================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:25
+
+  encodeULEB128(1, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
----------------
You don't necessarily need this in this patch in isolation, but the patterns you're using to define your abbrevs are likely to be fairly repetitive. I'm assuming you're going to add new unit tests to test the error handling? If so, you may want to look at adding one or more basic generator functions to factor out the test setup (and in which case you may want to do it as part of this patch, or a prerequisite patch). We have similar stuff for the, admittedly more complex, debug info and debug line sections IIRC.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:44
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(void *));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
----------------
I'm not convinced by using `sizeof(void *)` for the address size, rather than hard-coding values of 4 and 8: the address size is the size of the address in the section data. This is independent of the machine that the test code is built on (i.e. a 32-bit machine should be able to read 64-bit addresses and vice versa).

If it's irrelevant to the test, then hard coding in a value of 4 or 8 is fine. Otherwise, you should test with both 4 and 8-byte addreses.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:52
+
+  auto *Abbrev1 = AbbrevSet.getAbbreviationDeclaration(1);
+  EXPECT_TRUE(Abbrev1);
----------------
I think the LLVM policy on `auto` implies this shouldn't be `auto`, since it's not obvious what the actual type is if you aren't familiar with the interface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151001