[Lldb-commits] [PATCH] D124110: Refactor protected virtual functions from SymbolFile into new SymbolFileActual class.

[Pavel Labath via Phabricator via lldb-commits]

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