[PATCH] D46707: [DWARF] Factor out a DWARFUnitHeader class. NFC

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h:238
 
-class DWARFUnit {
+class DWARFUnit : public DWARFUnitHeader {
   DWARFContext &Context;
----------------
clayborg wrote:
> dblaikie wrote:
> > Any chance of composition rather than inheritance? (I realize that'll be a bunch more churn (changing "getLength" to "getHeader().getLength", etc, presumably) - but it seems like the inheritance here isn't accurate - a DWARFUnit is not a DWARFUnitHeader (in the "isa" kind of phrasing/test about inheritance))
> > 
> > Guess that could be done in a separate patch to make it clearer.
> It should be easy as each constructor was taking a "const DWARFUnitHeader &Header" parameter already, so instead of constructing DWARFUnitHeader with it, we could copy or move it into the m_header ivar.
Yeah, I wondered about composition versus inheritance.  If I throw in DWARFUnit methods that forward to the Header member then it would be less churn.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp:278
+      DWARFUnitHeader Header;
+      Header.extract(DCtx, DebugInfoData, &OffsetStart);
       std::unique_ptr<DWARFUnit> Unit;
----------------
dblaikie wrote:
> Rather than testing the section inside DWARFUnitHeader::extract - perhaps you could pass down UnitType here?
Not 100% sure what you're suggesting--yes in this path we do already have the UnitType, but in the non-verifier path we don't, so passing in the UnitType feels like adding unnecessary complexity to extract().


Repository:
  rL LLVM

https://reviews.llvm.org/D46707