labath added a comment.
In D124110#3463380 <https://reviews.llvm.org/D124110#3463380>, @clayborg wrote:
> I might suggest we rename things like suggested in my inline comment, and
> then have the "SymbolFile.h" class just include the extra needed header file?
> Many of the changes in this diff would just go away.
Those would be consistent with some existing libraries (e.g. those which use
the `I` prefix to denote pure interfaces), but if the goal is to reduce the
number of changes, then I don't think it will help, as pretty much everything
should still refer to the objects through the interface name. The name
SymbolFileActual should pretty much only be used in the class declaration. but
there are many more places that one would need to change from SymbolFile to
SymbolFileInterface.
Instead of the name `Actual`, I might go for Common -- I'm thinking of the
class as something that contains implementations "common" to many (but not
necessarily all) symbol file classes.
> I am not a fan of the SymbolFileActual class nor having to switch to
> SymbolFileActual.h. I would almost prefer both SymbolFileActual and
> SymbolFile to stay in SymbolFile.h.
Having SymbolFile.h include the other header would be strange, as the include
would have to go to the bottom of the file. I did something like that recently,
but only as a temporary transition aid.
However, I don't have a problem with having both classes be defined in the same
header file. I don't know if there's any direct reference from
SymbolFileOnDemand to SymbolFileActual. Ideally there wouldn't be, but if there
is, then that class would also have to be defined in the same file -- also not
a tragedy if the file does not get too big.
================
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:133-134
// Compile Unit function calls
// Approach 1 - iterator
- uint32_t GetNumCompileUnits();
- lldb::CompUnitSP GetCompileUnitAtIndex(uint32_t idx);
+ // Make virtual so that SymbolFileOnDemand can override.
+ virtual uint32_t GetNumCompileUnits() = 0;
----------------
The comment probably not necessary. In this setup, there's no way that a
non-virtual method can make sense, as there is nothing one could use to
implement it.
================
Comment at: lldb/include/lldb/Symbol/SymbolFile.h:371-379
lldb::ObjectFileSP m_objfile_sp; // Keep a reference to the object file in
// case it isn't the same as the module
// object file (debug symbols in a separate
// file)
- llvm::Optional<std::vector<lldb::CompUnitSP>> m_compile_units;
- TypeList m_type_list;
Symtab *m_symtab = nullptr;
uint32_t m_abilities = 0;
bool m_calculated_abilities = false;
----------------
What's up with the rest of the members. Can we move these too?
Any member that stays here runs the risk of going out of sync when we have two
instances floating around.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124110/new/
https://reviews.llvm.org/D124110
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
