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

[lattner]

Summary

PreprocessorTracker is the last user of the old StringPool class, which
isn't super loved and isn't a great improvement over a plan StringSet.
Once this goes in we can remove StringPool entirely.

This is as discussed on cfe-dev.

[MaskRay]

clang-tools-extra/modularize/PreprocessorTracker.cpp

912

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.

[lattner]

Hey @MaskRay, yes StringSet/StringMap's entries are separately allocated and the iterators/pointers are stable. The strings are held in each entry.

[rnk]

lgtm

clang-tools-extra/modularize/PreprocessorTracker.cpp

466

Nice.

912

I am only one data point, but yes, I knew that before this came up. :)

[lattner]

Thank you for the review Reid!

clang-tools-extra/modularize/PreprocessorTracker.cpp

 // With the map of all the macro and conditional expansion instances,
 // it might be possible to add to the existing modularize error messages
 // (the second part referring to definitions being different), attempting
 // to tie them to the last macro conflict encountered with respect to the
 // order of the code encountered.
 //
 //===--------------------------------------------------------------------===//
 
-#include "clang/Lex/LexDiagnostic.h"
 #include "PreprocessorTracker.h"
+#include "ModularizeUtilities.h"
+#include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "llvm/ADT/SmallSet.h"
-#include "llvm/Support/StringPool.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/raw_ostream.h"
-#include "ModularizeUtilities.h"
 
 namespace Modularize {
 
 // Some handle types
-typedef llvm::PooledStringPtr StringHandle;
+typedef llvm::StringRef StringHandle;
 
 typedef int HeaderHandle;
 const HeaderHandle HeaderHandleInvalid = -1;
 
 typedef int InclusionPathHandle;
 const InclusionPathHandle InclusionPathHandleInvalid = -1;
 
 // Some utility functions.

 namespace {
 
 // ConditionValueKind strings.
 const char *
 ConditionValueKindStrings[] = {
   "(not evaluated)", "false", "true"
 };
 
-bool operator<(const StringHandle &H1, const StringHandle &H2) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff < 0;
-}
-bool operator>(const StringHandle &H1, const StringHandle &H2) {
-  const char *S1 = (H1 ? *H1 : "");
-  const char *S2 = (H2 ? *H2 : "");
-  int Diff = strcmp(S1, S2);
-  return Diff > 0;
-}
-
 // Preprocessor item key.
 //
 // This class represents a location in a source file, for use
 // as a key representing a unique name/file/line/column quadruplet,
 // which in this case is used to identify a macro expansion instance,
 // but could be used for other things as well.
 // The file is a header file handle, the line is a line number,
 // and the column is a column number.
if (isHeaderHandleInStack(H)) {
         TH = getCurrentHeaderHandle();
         popHeaderHandle();
       } while ((TH != H) && (HeaderStack.size() != 0));
     }
     InNestedHeader = false;
   }
 
   // Lookup/add string.
-  StringHandle addString(llvm::StringRef Str) { return Strings.intern(Str); }
+  StringHandle addString(llvm::StringRef Str) {
+    return Strings.insert(Str).first->first();
+  }
 
   // Convert to a canonical path.
   std::string getCanonicalPath(llvm::StringRef path) const {
     std::string CanonicalPath(path);
     std::replace(CanonicalPath.begin(), CanonicalPath.end(), '\\', '/');
     return CanonicalPath;
   }
 
public:
 
   // Get the handle of a header file entry.
   // Return HeaderHandleInvalid if not found.
   HeaderHandle findHeaderHandle(llvm::StringRef HeaderPath) const {
     std::string CanonicalPath = getCanonicalPath(HeaderPath);
     HeaderHandle H = 0;
     for (auto I = HeaderPaths.begin(), E = HeaderPaths.end(); I != E;
          ++I, ++H) {
-      if (**I == CanonicalPath)
+      if (*I == CanonicalPath)
         return H;
     }
     return HeaderHandleInvalid;
   }
 
   // Add a new header file entry, or return existing handle.
   // Return the header handle.
   HeaderHandle addHeader(llvm::StringRef HeaderPath) {
for (auto I = MacroExpansions.begin(), E = MacroExpansions.end(); I != E;
       const PPItemKey &ItemKey = I->first;
       MacroExpansionTracker &MacroExpTracker = I->second;
       // If no mismatch (only one instance value) continue.
       if (!MacroExpTracker.hasMismatch())
         continue;
       // Tell caller we found one or more errors.
       ReturnValue = true;
       // Start the error message.
-      OS << *MacroExpTracker.InstanceSourceLine;
+      OS << MacroExpTracker.InstanceSourceLine;
       if (ItemKey.Column > 0)
         OS << std::string(ItemKey.Column - 1, ' ') << "^\n";
-      OS << "error: Macro instance '" << *MacroExpTracker.MacroUnexpanded
+      OS << "error: Macro instance '" << MacroExpTracker.MacroUnexpanded
          << "' has different values in this header, depending on how it was "
             "included.\n";
       // Walk all the instances.
       for (auto IMT = MacroExpTracker.MacroExpansionInstances.begin(),
                 EMT = MacroExpTracker.MacroExpansionInstances.end();
            IMT != EMT; ++IMT) {
         MacroExpansionInstance &MacroInfo = *IMT;
-        OS << "  '" << *MacroExpTracker.MacroUnexpanded << "' expanded to: '"
-           << *MacroInfo.MacroExpanded
+        OS << "  '" << MacroExpTracker.MacroUnexpanded << "' expanded to: '"
+           << MacroInfo.MacroExpanded
            << "' with respect to these inclusion paths:\n";
         // Walk all the inclusion path hierarchies.
         for (auto IIP = MacroInfo.InclusionPathHandles.begin(),
                   EIP = MacroInfo.InclusionPathHandles.end();
              IIP != EIP; ++IIP) {
           const std::vector<HeaderHandle> &ip = getInclusionPath(*IIP);
           auto Count = (int)ip.size();
           for (int Index = 0; Index < Count; ++Index) {
             HeaderHandle H = ip[Index];
-            OS << std::string((Index * 2) + 4, ' ') << *getHeaderFilePath(H)
+            OS << std::string((Index * 2) + 4, ' ') << getHeaderFilePath(H)
                << "\n";
           }
         }
         // For a macro that wasn't defined, we flag it by using the
         // instance location.
         // If there is a definition...
         if (MacroInfo.DefinitionLocation.Line != ItemKey.Line) {
-          OS << *MacroInfo.DefinitionSourceLine;
+          OS << MacroInfo.DefinitionSourceLine;
           if (MacroInfo.DefinitionLocation.Column > 0)
             OS << std::string(MacroInfo.DefinitionLocation.Column - 1, ' ')
                << "^\n";
           OS << "Macro defined here.\n";
         } else
           OS << "(no macro definition)"
              << "\n";
       }
for (auto I = ConditionalExpansions.begin(),
          I != E; ++I) {
       const PPItemKey &ItemKey = I->first;
       ConditionalTracker &CondTracker = I->second;
       if (!CondTracker.hasMismatch())
         continue;
       // Tell caller we found one or more errors.
       ReturnValue = true;
       // Start the error message.
-      OS << *HeaderPaths[ItemKey.File] << ":" << ItemKey.Line << ":"
+      OS << HeaderPaths[ItemKey.File] << ":" << ItemKey.Line << ":"
          << ItemKey.Column << "\n";
       OS << "#" << getDirectiveSpelling(CondTracker.DirectiveKind) << " "
-         << *CondTracker.ConditionUnexpanded << "\n";
+         << CondTracker.ConditionUnexpanded << "\n";
       OS << "^\n";
       OS << "error: Conditional expression instance '"
-         << *CondTracker.ConditionUnexpanded
+         << CondTracker.ConditionUnexpanded
          << "' has different values in this header, depending on how it was "
             "included.\n";
       // Walk all the instances.
       for (auto IMT = CondTracker.ConditionalExpansionInstances.begin(),
                 EMT = CondTracker.ConditionalExpansionInstances.end();
            IMT != EMT; ++IMT) {
         ConditionalExpansionInstance &MacroInfo = *IMT;
-        OS << "  '" << *CondTracker.ConditionUnexpanded << "' expanded to: '"
+        OS << "  '" << CondTracker.ConditionUnexpanded << "' expanded to: '"
            << ConditionValueKindStrings[MacroInfo.ConditionValue]
            << "' with respect to these inclusion paths:\n";
         // Walk all the inclusion path hierarchies.
         for (auto IIP = MacroInfo.InclusionPathHandles.begin(),
                   EIP = MacroInfo.InclusionPathHandles.end();
              IIP != EIP; ++IIP) {
           const std::vector<HeaderHandle> &ip = getInclusionPath(*IIP);
           auto Count = (int)ip.size();
           for (int Index = 0; Index < Count; ++Index) {
             HeaderHandle H = ip[Index];
-            OS << std::string((Index * 2) + 4, ' ') << *getHeaderFilePath(H)
+            OS << std::string((Index * 2) + 4, ' ') << getHeaderFilePath(H)
                << "\n";
           }
         }
       }
     }
     return ReturnValue;
   }
 
default:
       return "(unknown)";
     }
   }
 
 private:
   llvm::SmallVector<std::string, 32> HeaderList;
   // Only do extern, namespace check for headers in HeaderList.
   bool BlockCheckHeaderListOnly;
-  llvm::StringPool Strings;
+  llvm::StringSet<> Strings;
   std::vector<StringHandle> HeaderPaths;
   std::vector<HeaderHandle> HeaderStack;
   std::vector<HeaderInclusionPath> InclusionPaths;
   InclusionPathHandle CurrentInclusionPathHandle;
   llvm::SmallSet<HeaderHandle, 32> HeadersInThisCompile;
   std::vector<PPItemKey> IncludeDirectives;
   MacroExpansionMap MacroExpansions;
   ConditionalExpansionMap ConditionalExpansions;