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

[David Blaikie via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:64
+public:
+  virtual ~FormSizeHelper() {}
+  virtual Optional<uint8_t> getAddressByteSize() const = 0;
----------------
Maybe '= default' rather than {}, though I don't think it makes a difference.

Also probably best to = delete the copy/move ops (the copy ops are the only ones that are a real concern, the move ones are already implicitly not provided due to the presence of this user declared dtor) to avoid any risk of slicing.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:78
+  FormSizeHelperDWARFUnit(const DWARFUnit *DU) : U(DU) {}
+  ~FormSizeHelperDWARFUnit() override {}
+  Optional<uint8_t> getAddressByteSize() const override {
----------------
Remove this - it's implied/the default.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:106
+      : Version(V), AddrSize(A), Format(F) {}
+  ~FormSizeHelperManual() override {}
+  Optional<uint8_t> getAddressByteSize() const override { return AddrSize; }
----------------
This is the implicit default, you can omit it entirely.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:120
+    }
+    return None;
+  }
----------------
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)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:124
+
+Optional<uint8_t> getFixedByteSize(dwarf::Form form,
+                                   const ::FormSizeHelper &FSH) {
----------------
LLVM style is to mark file-local functions as 'static' rather than placing them in an anonymous namespace (but classes, like those above, go in the anonymous namespace as they are).


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:191
+    llvm_unreachable("Handle this form in this switch statement");
+    return None;
+  }
----------------
skip the return after unreachable - it should be flagged as unreachable/dead by some compiler warnings


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:293
+  return ::getFixedByteSize(form,
+                            FormSizeHelperManual(Version, AddrSize, Format));
+}
----------------
Oh, I see - this type doesn't actually need a virtual dtor (it's never destroyed polymorphically - it could just have a protected dtor in the base class, and make derived classes final (so there's no possibility of accidental polymorphic ownership/destruction).

Also - perhaps we could just make getFixedByteSize a template (templated on the FSH type) & avoid the virtual calls? (remove the base class, and just have FormSizeHelper* have non-virtual functions but otherwise the same as they are today)

But maybe this polymorphism (static or dynamic) isn't really necessary - maybe getFixedByteSize needs a struct with the 3 values (FormSizeHelperManual, basically) & just a way to construct FormSizeHelperManual from a DWARFUnit? Is it worth delaying querying the DWARFUnit until the implementation of getFixedByteSize compared to just calling it at the call-site to getFixedByteSize?


https://reviews.llvm.org/D26526