[PATCH] D110196: [IR] Modernize and Cleanup LLVM/IR folder

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

Just a couple of comments inline to keep in mind if/when you start splitting pieces of this out.



================
Comment at: llvm/lib/IR/Function.cpp:692-695
+void Function::setGC(const std::string &Str) {
   setValueSubclassDataBit(14, !Str.empty());
   getContext().setGC(*this, std::move(Str));
 }
----------------
Is this an improvement? With this change (and the one to `LLVMContext::setGC()`), there's potentially allocation traffic with this usage pattern:
```
lang=c++
std::string GC = /* ... */;
F.setGC(std::move(GC));
```
where previously there was none.

Another option might be to make this (and the called API) a `StringRef`, and audit callers to delay any std::string allocation until `LLVMContext::setGC()`.


================
Comment at: llvm/lib/IR/IRPrintingPasses.cpp:55-56
 PrintFunctionPass::PrintFunctionPass() : OS(dbgs()) {}
-PrintFunctionPass::PrintFunctionPass(raw_ostream &OS, const std::string &Banner)
-    : OS(OS), Banner(Banner) {}
+PrintFunctionPass::PrintFunctionPass(raw_ostream &OS, std::string Banner)
+    : OS(OS), Banner(std::move(Banner)) {}
 
----------------
When you split this out, I'd suggest using `StringRef` as the parameter and then `Banner(Banner.str())`.


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

https://reviews.llvm.org/D110196