[PATCH] D46707: [DWARF] Factor out a DWARFUnitHeader class. NFC
Paul Robinson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 11 06:44:46 PDT 2018
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
More information about the llvm-commits
mailing list