[PATCH] D143550: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames (Deeper Refactoring)

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:324
   ValueSymbolTable *ST;
   if (getSymTab(this, ST))
     return;  // Cannot set a name on this value (e.g. constant).
----------------
Is there some reason to move the call to getSymTab() earlier?


================
Comment at: llvm/lib/IR/Value.cpp:334
+  if ((!getContext().shouldDiscardValueNames() || isa<GlobalValue>(this)) &&
+      !NewName.isTriviallyEmpty()) {
+    NameRef = NewName.toStringRef(NameData);
----------------
Why do we want to check isTriviallyEmpty() here?  Is this related to the earlier isTriviallyEmpty() check somehow?


================
Comment at: llvm/lib/IR/Value.cpp:344
 
-    // NOTE: Could optimize for the case the name is shrinking to not deallocate
-    // then reallocated.
-    destroyValueName();
+  assert((NameRef.empty() || !getType()->isVoidTy()) &&
+         "Cannot assign a name to void values!");
----------------
Not sure why you're messing with this assertion.  A void value doesn't have a name, and never had a name (unless you're doing something weird with mutating the types of values, which isn't relevant here).


================
Comment at: llvm/lib/IR/Value.cpp:359
   // NOTE: Could optimize for the case the name is shrinking to not deallocate
   // then reallocated.
   if (hasName()) {
----------------
I think the reason the way the code was structured this way is because of this "NOTE"... if you restructure this way, probably delete the NOTE.


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

https://reviews.llvm.org/D143550