[PATCH] D78273: [clang-tools-extra] reimplement PreprocessorTracker in terms of StringSet.

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: clang-tools-extra/modularize/PreprocessorTracker.cpp:466
 
-bool operator<(const StringHandle &H1, const StringHandle &H2) {
-  const char *S1 = (H1 ? *H1 : "");
----------------
Nice.


================
Comment at: clang-tools-extra/modularize/PreprocessorTracker.cpp:912
   // Lookup/add string.
-  StringHandle addString(llvm::StringRef Str) { return Strings.intern(Str); }
+  StringHandle addString(llvm::StringRef Str) {
+    return Strings.insert(Str).first->first();
----------------
MaskRay wrote:
> Is it well-known that a StringSet (= `StringMap<NoneType, ...>`) returned StringRef is stable? Is that property something we can reliably depend on?
> 
> If not, StringSaver.h:UniqueStringSaver may be a better choice.
I am only one data point, but yes, I knew that before this came up. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78273