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

[Lang Hames via Phabricator via llvm-commits]

lhames marked 2 inline comments as done.
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:
> 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? 
Yep -- that's the approach used in the latest patch.


================
Comment at: llvm/unittests/ExecutionEngine/Orc/SymbolStringPoolTest.cpp:32
+TEST_F(SymbolStringPoolTest, UniquingAndComparisons) {
   SymbolStringPool SP;
   auto P1 = SP.intern("hello");
----------------
dblaikie wrote:
> Do you need these locals anymore, now that you have an `SP` member?
Nope -- just missed those lines in the cleanup. Thanks for catching that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142314