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

[Adrian Prantl via llvm-commits]

> On Nov 10, 2016, at 3:33 PM, Adrian Prantl <aprantl at apple.com> wrote:
> 
> 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)

I just noticed I had an in-flight collision with David's comment about laziness. I'll leave it up to you two to come up with a reasonable design before landing this.

-- adrian
> 
> 
> 
> ================
> 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
> 
> 
>