[PATCH] D132988: Changes to use a string pool for symbols

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:500
   /// Returns the name of this symbol (empty if the symbol is anonymous).
-  StringRef getName() const {
-    assert((!Name.empty() || getScope() == Scope::Local) &&
+  orc::SymbolStringPtr getName() const {
+    assert((Name || getScope() == Scope::Local) &&
----------------
jaredwy wrote:
> dblaikie wrote:
> > This could return by const-ref (the same way a large member like a std::vector would return from an accessor by const ref) & then `isNamed` could be removed & the usage code kept the same? (I assume `isNamed` was added to avoid the cost of copying the `SymbolStringPtr` - and if it returns by const ref there's no added cost? Oh, I guess callers still have to add the `*` in to dereference? yeah, but still maybe better than adding this `isNamed` function that's a bit quirky - since it only allows the one operation (equality) without the `SymbolStringPtr` copy, which is a bit niche/special case/inflexible)
> It was mostly added to aid in readability of the code and avoiding the unnecessary copy but I am not attached to it, I am new to these lands so if it is not the LLVM way to add utility functions i can remove
Could you show/compare the readability issues you had in mind?

Would `getName` returning by const-ref address the unnecessary copy concerns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132988