[PATCH] D26526: Clean up DWARFFormValue by reducing duplicated code and removing DWARFFormValue::getFixedFormSizes()

[Adrian Prantl via llvm-commits]

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

Thanks, I think this is looking good now!
(with outstanding inline comments addressed)



================
Comment at: include/llvm/Support/Dwarf.h:436
 
+// Constants that define the DWARF format as 32 or 64 bit.
+enum DwarfFormat { DWARF32, DWARF64 };
----------------
///


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:61
+// is needed because we have two clients: ones that passed in a DWARFUnit, and
+// ones that pass in a version, address byte size and a DWARFFormat.
+class FormSizeHelper {
----------------
///


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:97
+// FormSizeHelperManual enables us to determine the sizes of some forms using
+// the supplied Version, AddrSize and Format.
+class FormSizeHelperManual : public FormSizeHelper {
----------------
///


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:196
+
+static bool skipFormValue(dwarf::Form form, const DataExtractor &DebugInfoData,
+                   uint32_t *offset_ptr, const FormSizeHelper &FSH) {
----------------
`Form`


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:197
+static bool skipFormValue(dwarf::Form form, const DataExtractor &DebugInfoData,
+                   uint32_t *offset_ptr, const FormSizeHelper &FSH) {
+  bool indirect = false;
----------------
`OffsetPointer` or `OffsetPtr`


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:198
+                   uint32_t *offset_ptr, const FormSizeHelper &FSH) {
+  bool indirect = false;
+  do {
----------------
`Indirect`


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:202
+    // Blocks if inlined data that have a length field and the data bytes
+    // inlined in the .debug_info
+    case DW_FORM_exprloc:
----------------
.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:253
+
+    // signed or unsigned LEB 128 values
+    case DW_FORM_sdata:
----------------
S ... .


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:136
   DataExtractor InfoData(Info, true, 0);
-  InfoData.getU32(&Offset); // Length
+  llvm::dwarf::DwarfFormat Format = llvm::dwarf::DwarfFormat::DWARF32;
+  uint64_t Length = InfoData.getU32(&Offset);
----------------
I think this is file might be using the llvm namespace already?


================
Comment at: unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp:27
+  Optional<uint8_t> AddrSize;
+  // Test 32 bit DWARF version 2 with 4 byte addresses
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 4, DWARF32);
----------------
.


================
Comment at: unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp:34
+
+  // Test 32 bit DWARF version 2 with 8 byte addresses
+  RefSize = DWARFFormValue::getFixedByteSize(DW_FORM_ref_addr, 2, 8, DWARF32);
----------------
.


https://reviews.llvm.org/D26526