[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[jeffrey tan via Phabricator via lldb-commits]

yinghuitan added a comment.

Thanks for clarifying. The friendship declaration in SymbolFile is required not only for `SetCompileUnitAtIndex` but also for other protected virtual methods in `SymbolFile` like `CalculateNumCompileUnits` and `ParseCompileUnitAtIndex` which are pure virtual and require overriding and forwarding by `SymbolFileOnDemand`. 
One solution I can think of is creating a new `SymbolFileActual` class as child class of `SymbolFile` and move all the protected virtual member functions from `SymbolFile` into `SymbolFileActual`, then modify the callsites of these protected virtual member functions into `SymbolFileActual` as well. This will get rid of friend declaration and SymbolFile won't/can't have protected virtual member functions. Something like below:

  // Only has public virtual functions
  class SymbolFile {
  };
  
  // Override and forward the public virtual functions so no friend is needed
  class SymbolFileOnDemand: public SymbolFile {}
  
  class SymbolFileActual: public SymbolFile {
  public:
    // Override any public virtual functions of SymbolFile 
    // that are callsites of protected virtual functions
  protected:
    // All protected virtual function of SymbolFile are moved here
    // All member variables of SymbolFile are moved here
  };
  
  class SymbolFileDWARF: public SymbolFileActual {}
  class SymbolFilePDB: public SymbolFileActual {}
  class SymbolFileBreakPad: public SymbolFileActual {}
  ...

Is this what you are suggesting? This seems to be a big change which might introduce new issues so I took a more practical approach with minimum change instead here. Let me know if this `SymbolFileActual` design aligns with what you have in mind, and if so whether you want it to happen in this diff or a follow-up patch is fine. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121631/new/

https://reviews.llvm.org/D121631