[Lldb-commits] [PATCH] D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit

[via lldb-commits]

> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf
> Of Jan Kratochvil via llvm-commits
> Sent: Friday, April 06, 2018 5:29 PM
> To: Greg Clayton
> Cc: reviews+D45170+public+e75ed5903a857119 at reviews.llvm.org; Jason
> Molenda; lldb-commits at lists.llvm.org; llvm-commits; Davide Italiano
> Subject: Re: [PATCH] D45170: Cleanup DWARFCompileUnit and DWARFUnit in
> preparation for adding DWARFTypeUnit
> 
> On Fri, 06 Apr 2018 22:52:05 +0200, Greg Clayton wrote:
> > Switching over to the LLVM parser will require some detailed work and
> will
> > take some time.
> 
> So should I continue pushing the DWZ patchset even before the reuse of
> LLVM
> DWARFUnit happens?
> 
> 
> > That being said, I am confused as to why this was reverted. The code I
> added
> > mirrors the LLVM code a bit better, and yes it will require some
> reworking
> > of your patches.
> 
> Planned DWARFPartialUnit needs the inheritance as it was.  As you have
> moved
> most of the data fields back to DWARFUnit I will need to create some new
> superclass DWARFSomeNameUnit and move back the data fields from DWARFUnit
> to
> DWARFSomeNameUnit to get:
> 	DWARFUnit->DWARFSomeNameUnit->DWARFCompileUnit
> 	DWARFUnit->DWARFSomeNameUnit->DWARFTypeUnit
> 	DWARFUnit->DWARFPartialUnit

I have to say I am a little surprised by this hierarchy.  I would have
thought type units and partial units would be more alike.  Type units
are imported by signature not reference but otherwise I'd think they
would have a lot in common: lumps of DWARF that aren't their own
compilation unit.
--paulr

> 
> If you expected my DWARFUnit will serve as a superclass for
> DWARFCompileUnit+DWARFTypeUnit then you should not have approved it the
> way I
> wrote it as my goal was DWARFPartialUnit which has very different
> inheritance
> requirements than DWARFTypeUnit.
> 
> 
> > The DWARFUnit having an accessor to give out
> > a DWARFCompileUnit was really confusing and not the right layering.
> 
> This is how DWARFPartialUnit works, it is only a DWARFCompileUnit remapped
> to
> new offset.  I do not see how to implement it transparently without the
> accessor (and without needlessly copying all the data fields many times
> into
> each DWARFPartialUnit instance).
> 
> 
> > So I fixed the layering. I need to submit .debug_types patches and that
> > patch was needed for this and now I am back to square one.
> 
> I will sure deal with your reverting of my revert.  I just do not like the
> words "fixes the layering" as it rather "changes the layering" for
> different
> purposes than my DWARFUnit was implemented for.
> 
> 
> Jan
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits