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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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
----------------
lhames wrote:
> 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
> ```
> 
Ah, fair enough - sounds good :)

Hmm, could the debugging/testing use cases be covered by the private member anyway? 


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:248
 
+#ifndef NDEBUG
+inline size_t
----------------
Hmm, wouldn't this be non-NDEBUG conditional? (since it's declared unconditionally and called unconditionally from the test code now?)


================
Comment at: llvm/unittests/ExecutionEngine/Orc/SymbolStringPoolTest.cpp:32
+TEST_F(SymbolStringPoolTest, UniquingAndComparisons) {
   SymbolStringPool SP;
   auto P1 = SP.intern("hello");
----------------
Do you need these locals anymore, now that you have an `SP` member?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142314