[PATCH] D143487: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames

Tue Mar 21 13:21:29 PDT 2023

efriedma added inline comments.

================
Comment at: llvm/lib/IR/Value.cpp:323
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&
----------------
yrouban wrote:
> efriedma wrote:
> > Can we just make this `StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";`,instead of messing with the rest of the function?
> I agree that we have to avoid //NewName.toStringRef(NameData)//. I have another patch that refactors //Value::setNameImpl()// further in this way - D143550. Please take a look and choose one patch that I should finalize.
Yes, D143550 does that... but it also rearranges the whole function in a bunch of other ways that I'm not sure make sense.  I'd prefer the minimal change for the bugfix, then considering the full refactoring separately.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143487