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

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: llvm/include/llvm/IR/ValueSymbolTable.h:78
+    // lookup will fail if Name exceeds the size cap.
+    return vmap.lookup(capLocalValueName(Name));
+  }
----------------
hasyimibhar wrote:
> mehdi_amini wrote:
> > The fact that the table lookups would automatically use this but not the insertions looks like a red flag to me. If this is an invariant of the table then it should be private the table and reflected by the API consistently.
> I moved NonGlobalValueMaxNameSize into ValueSymbolTable, and made it responsible for doing the capping. It looks like the updated name will be propagated back to the Value class (since createValueName is called by Value::setName), so it should be fine to move it.
> 
> I also added isLocal argument to ValueSymbolTable::lookup, because it seems that it's also used to look up global value in LLParser, so there must be a way to differentiate between global and local value name lookup.

> I also added isLocal argument to ValueSymbolTable::lookup, because it seems that it's also used to look up global value in LLParser, so there must be a way to differentiate between global and local value name lookup.

That's why I suggested to create a type adaptor: a type,  that derives from ValueSymbolTable




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707