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

[Greg Clayton via llvm-commits]

clayborg marked 6 inline comments as done.
clayborg added a comment.

Marked things as done. Let me know if this is enough or if you have objections to the FormSizeHelper class. I think it is better than extracting 3 values that you are almost 90% of the time never use. The construction of FormSizeHelperDWARFUnit is very cheap and it will be 2 pointers in size so it shouldn't be too bad.



================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:120
+    }
+    return None;
+  }
----------------
dblaikie wrote:
> should probably be llvm_unreachable (LLVM might flag this code as unreachable, since the above switch is fully covering all the enum values - and we'll have a warning/error if a new enum value is added that's not covered in the switch)
Yeah, that is why I left it out as I figured the warning for a missing value would show up, but I can add an llvm_unreachable here.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:292-293
+                                 llvm::dwarf::DwarfFormat Format) {
+  return ::getFixedByteSize(form,
+                            FormSizeHelperManual(Version, AddrSize, Format));
+}
----------------
Yeah I thought about these issues. I didn't want to always extract 3 values for each form we are going to skip since a large percent of the forms will never touch or access the data in the first place. So I kind of like the laziness of the current implementation because most forms don't need to access the DWARFUnit or the Version/AddrSize/Format.

I can remove the virtual destructor and declare the two classes final though. You ok with that?


https://reviews.llvm.org/D26526