[PATCH] D102707: Fix non-global-value-max-name-size not considered by LLParser

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/include/llvm/IR/ValueSymbolTable.h:74
   /// Lookup a named Value.
-  Value *lookup(StringRef Name) const { return vmap.lookup(Name); }
+  virtual Value *lookup(StringRef Name) const;
 
----------------
serge-sans-paille wrote:
> @mehdi_amini are we fine with making a few methods here virtual? The approach proposed by @hasyimibhar is elegant, but I'm worried about the performance impact.
> 
> An alternative would be to have something like (pseudo code)
> 
> ```
> class LocalValueSymbolTable {
>     ValueSymbolTable table;
>     public:
>       using ValueSymbolTable::size;  // nothing to change for that one
>       Value *lookup(StringRef Name) const { // cap then forward to table }
>     private:
>       ValueName *createValueName(StringRef Name, Value *V); { // cap then forward to table }
> ```
>       
>       
+1: let's not make virtual if we can have an adapter class.

Another way to do it is by refactoring the base class for the common methods, and two derived class that implement the specific methods for each case. You only need virtual for the derived method if you actually need to use this class polymorphically (do you?).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707