[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) &&
----------------
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)


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:503-505
+    if (!Name)
+      return nullptr;
     return Name;
----------------
I think this can be removed now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132988