[PATCH] D142314: [ORC] Add a NonOwningSymbolStringPtr utility.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 24 22:30:34 PST 2023
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
More information about the llvm-commits
mailing list