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

[Jared Wyles via Phabricator via llvm-commits]

jaredwy added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:418
   static Symbol &constructCommon(BumpPtrAllocator &Allocator, Block &Base,
-                                 StringRef Name, orc::ExecutorAddrDiff Size,
-                                 Scope S, bool IsLive) {
-    assert(!Name.empty() && "Common symbol name cannot be empty");
+                                 orc::SymbolStringPtr &&Name,
+                                 orc::ExecutorAddrDiff Size, Scope S,
----------------
sgraenitz wrote:
> Is there a special reason for these methods to take RValue references? Otherwise, these look like classical sink arguments that should be moved in and taken by value (just remove the `&&`).
No not really and in fact started life that way, just was looking to avoid the overhead of the add/decrement of the ownership counter when I noticed every usage was taking ownership, will remove :) 


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:973
   LinkGraph &operator=(LinkGraph &&) = delete;
+  ~LinkGraph();
 
----------------
sgraenitz wrote:
> Do we have a definition for the dtor here?
Sorry don't follow the question? Implementation is further down if that's what you meant?  Did you want me to move it inline here? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132988