[PATCH] D142314: [ORC] Add a NonOwningSymbolStringPtr utility.

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:50-54
+#ifndef NDEBUG
+  // Useful for debugging and testing: This method can be used to identify
+  // non-owning pointers pointing to unowned pool entries.
+  size_t getRefCount(const SymbolStringPtrBase &S) const;
+#endif
----------------
dblaikie wrote:
> Could this be private and friended to the test class, rather than only in +Asserts? (that way it'd be tested in non-Asserts, and not callable from code/accidentally used in +Asserts builds in the rest of the code)
Ahh -- I was thinking of using this in assertions (and the tests), but wanted to discourage use outside that. To get the best of both worlds I could take your suggestion and make `getRefCount` private, then introduce a new method:
```
#ifndef NDEBUG
  bool isPoolEntryValid() const { return getRefCount(); }
#endif
```



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:87-88
+
+  friend bool operator!=(const SymbolStringPtrBase &LHS,
+                         const SymbolStringPtrBase &RHS) {
+    return !(LHS == RHS);
----------------
dblaikie wrote:
> If these things are only `sizeof(void*)` then maybe pass by value, rather than const ref?
They could be pass by value. Is there likely to be any benefit on a trivially inline-able method like this? Or is it mostly a style issue? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142314