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

[Greg Clayton via lldb-commits]

> On Apr 6, 2018, at 2:29 PM, Jan Kratochvil <jan.kratochvil at redhat.com> wrote:
> 
> 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
> 
> 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.

It was approved, but I was looking at one patch at a time and didn't look forward at any other patches since I already noticed things would need to change in future patches. Take look at how LLVM does it. I believe my changes mirror that approach. DWARFUnit should be the base class for anything that needs to hand out DIEs. Any specializations should be inheriting from DWARFUnit, like both DWARFCompileUnit and DWARFTypeUnit. I have no idea what are in your other patches and it was patch overload and no one can track anything in those patches as they are too much to process without looking at the first patches. So that is what we are doing here is going through your patches one by one and they will need to change as things change.

> 
> 
>> 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).

What extra functions are needed for a partial unit that can't be done in a DWARFCompileUnit? Seems like they both contain things, but the partial unit can be referenced from compile units. 
> 
> 
> 
>> 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.

As we are saying, we are trying to make the layering more like LLVM's layering, so that is what I meant by "fix the layering". I believe we should strive for being more like LLVM so that any transition can happen without major re-organization of the DWARF code later. So I would like the get the ok the revert the revert if you on board with what my suggestions are in the paragraph. I know this will require modifications to your patches and apologize for that.

Let me know what you think,

Greg Clayton