[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Take a look how the LLVM DWARF parser handles its units. It makes a DWARFUnit base class that the compile unit inherits from. That can then be used for type units and compile units.



================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:23-32
+class DWARFCompileUnitDecls {
 public:
   enum Producer {
     eProducerInvalid = 0,
     eProducerClang,
     eProducerGCC,
     eProducerLLVMGCC,
----------------
Just make this enum DWARFProducer and get rid of the DWARFCompileUnitDecls class.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:34
+
+class DWARFCompileUnitData : public DWARFCompileUnitDecls {
+public:
----------------
Remove DWARFCompileUnitDecls and just use DWARFProducer


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:34
+
+class DWARFCompileUnitData : public DWARFCompileUnitDecls {
+public:
----------------
clayborg wrote:
> Remove DWARFCompileUnitDecls and just use DWARFProducer
The LLVM DWARF parser calls this class a DWARFUnit. We should probably do the same.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:77
 
+class DWARFCompileUnit : public DWARFCompileUnitDecls {
+public:
----------------
remove DWARFCompileUnitDecls and just use DWARFProducer


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216
 
+  DWARFCompileUnitData *m_data;
+
----------------
jankratochvil wrote:
> clayborg wrote:
> > Is there a reason this is a member variable that I am not seeing? Seems we could have this class inherit from DWARFCompileUnitData. I am guessing this will be needed for a future patch?
> Yes, future patch D40474 contains a new constructor so multiple `DWARFCompileUnit` then point to single `DWARFCompileUnitData`.  Sure that happens only in the case ot `DW_TAG_partial_unit` (one `DWARFCompileUnit` is read from a file while other `DWARFCompileUnit` are remapped instances with unique offset as used from units which did use `DW_TAG_imported_unit` for them).
> `DWARFCompileUnit(DWARFCompileUnitData *data, DWARFCompileUnit *main_cu);`
> 
We should just have an instance of this in this class and add a virtual function to retrieve it. Then we make a DWARFPartialUnit that subclasses this and has its own extra member variable and can use either one when it makes sense. Not a fan of just having a dangling pointer with no clear ownership. There is no "delete m_data" anywhere?


https://reviews.llvm.org/D40466