[ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

Cc John Thompson, but I don’t know his email address.

I recently ran across llvm::StringPool in llvm/lib/Support. It is an old class which isn’t ideal in several ways, and I would like to remove it in favor of direct uses of llvm::StringSet, which is nearly the same thing and a lot more efficient. The one difference is that in this one, each entry is individually reference counted, so they get deallocated as soon as the last reference is dropped.

The only use of this class is in PreprocessorTrackerImpl in clang-tools-extra/modularize/PreprocessorTracker.cpp. Does anyone object to changing this to being a simple llvm::StringSet? Such a change would make this more efficient (and allow deleting a misleading class out of llvm/lib/Support), allow simplifying away the "StringHandle” logic (in favor of using StringRef directly) and the only cost that I can see is that the strings won’t get deallocated as eagerly.

An alternative solution here is to move StringPool into clang-tools-extra as a private implementation detail. I’d personally rather remove it outright than preserve it.

Any thoughts?

-Chris

I also noticed StringPool last month and had more or less the same thought: this looks dead, can we remove it?

+1 for replacing this usage with StringSet and removing StringPool.

I also noticed StringPool last month and had more or less the same thought:
this looks dead, can we remove it?

+1 for replacing this usage with StringSet and removing StringPool.

+1. llvm/Support/StringSaver.h:UniqueStringSaver does more or less the
same thing.

Cool, thanks! I put up this patch to replace this usage in favor of StringSet - once this goes in, I’ll remove StringPool entirely. I’d appreciate an LGTM, thanks!

https://reviews.llvm.org/D78273

-Chris