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

[Alex Langford via Phabricator via llvm-commits]

bulbazord added inline comments.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:25
+
+  encodeULEB128(1, OS);
+  encodeULEB128(DW_TAG_compile_unit, OS);
----------------
jhenderson wrote:
> 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.
Yes, I'll be adding more tests, so yanking the setup into a separate function is appropriate. It probably does not need to be very complex though. I'll do so in this patch. Thanks for the suggestion.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp:44
+  uint64_t Offset = 0;
+  DataExtractor Data(RawData, sys::IsLittleEndianHost, sizeof(void *));
+  DWARFAbbreviationDeclarationSet AbbrevSet;
----------------
jhenderson wrote:
> 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.
It is indeed irrelevant to the to the test. I'll hardcode 8.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151001