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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
