NFC: Clean up the implementation of StringPool a bit, and remove dependence on some "implicitly MallocAllocator" based methods on StringMapEntry. This allows reducing the #includes in StringMapEntry.h.

[lattner]

Summary

StringPool has many caveats and isn't used in the monorepo. I will
propose removing it as a patch separate from this refactoring patch.

[lattner]

Update uses of the removed APIs in LLVMCore.

[lattner]

Update the StringSet/Map unit tests to stop using the removed APIs.

llvm/include/llvm/ADT/StringMap.h

 //===- StringMap.h - String Hash table map interface ------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines the StringMap class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_STRINGMAP_H
 #define LLVM_ADT_STRINGMAP_H
 
 #include "llvm/ADT/StringMapEntry.h"
+#include "llvm/Support/AllocatorBase.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 #include <initializer_list>
 #include <iterator>
 
 namespace llvm {
 
 template <typename ValueTy> class StringMapConstIterator;
 template <typename ValueTy> class StringMapIterator;

llvm/include/llvm/ADT/StringMapEntry.h

 // inclusion in public headers than StringMap.h itself is.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ADT_STRINGMAPENTRY_H
 #define LLVM_ADT_STRINGMAPENTRY_H
 
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/AllocatorBase.h"
 
 namespace llvm {
 
 /// StringMapEntryBase - Shared base class of StringMapEntry instances.
 class StringMapEntryBase {
   size_t keyLength;
 
 public:
static StringMapEntry *Create(StringRef key, AllocatorTy &allocator,
     // Copy the string information.
     char *strBuffer = const_cast<char *>(newItem->getKeyData());
     if (keyLength > 0)
       memcpy(strBuffer, key.data(), keyLength);
     strBuffer[keyLength] = 0; // Null terminate for convenience of clients.
     return newItem;
   }
 
-  /// Create - Create a StringMapEntry with normal malloc/free.
-  template <typename... InitType>
-  static StringMapEntry *Create(StringRef key, InitType &&... initVal) {
-    MallocAllocator allocator;
-    return Create(key, allocator, std::forward<InitType>(initVal)...);
-  }
-
-  static StringMapEntry *Create(StringRef key) {
-    return Create(key, ValueTy());
-  }
-
   /// GetStringMapEntryFromKeyData - Given key data that is known to be embedded
   /// into a StringMapEntry, return the StringMapEntry itself.
   static StringMapEntry &GetStringMapEntryFromKeyData(const char *keyData) {
     char *ptr = const_cast<char *>(keyData) - sizeof(StringMapEntry<ValueTy>);
     return *reinterpret_cast<StringMapEntry *>(ptr);
   }
 
   /// Destroy - Destroy this StringMapEntry, releasing memory back to the
   /// specified allocator.
   template <typename AllocatorTy> void Destroy(AllocatorTy &allocator) {
     // Free memory referenced by the item.
     size_t AllocSize = sizeof(StringMapEntry) + this->getKeyLength() + 1;
     this->~StringMapEntry();
     allocator.Deallocate(static_cast<void *>(this), AllocSize);
   }
-
-  /// Destroy this object, releasing memory back to the malloc allocator.
-  void Destroy() {
-    MallocAllocator allocator;
-    Destroy(allocator);
-  }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGMAPENTRY_H

llvm/include/llvm/Support/StringPool.h

-//===- StringPool.h - Interned string pool ----------------------*- C++ -*-===//
+//===- StringPool.h - Intern'd string pool ----------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// This file declares an interned string pool, which helps reduce the cost of
-// strings by using the same storage for identical strings.
+// This file declares an interned string pool with separately malloc and
+// reference counted entries.  This can reduce the cost of strings by using the
+// same storage for identical strings.
 //
 // To intern a string:
 //
 //   StringPool Pool;
 //   PooledStringPtr Str = Pool.intern("wakka wakka");
 //
 // To use the value of an interned string, use operator bool and operator*:
 //
 //   if (Str)
 //     cerr << "the string is" << *Str << "\n";
 //
 // Pooled strings are immutable, but you can change a PooledStringPtr to point
 // to another instance. So that interned strings can eventually be freed,
 // strings in the string pool are reference-counted (automatically).
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_STRINGPOOL_H
 #define LLVM_SUPPORT_STRINGPOOL_H
 
 #include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringRef.h"
-#include <cassert>
 
 namespace llvm {
 
-  class PooledStringPtr;
 
-  /// StringPool - An interned string pool. Use the intern method to add a
-  /// string. Strings are removed automatically as PooledStringPtrs are
-  /// destroyed.
-  class StringPool {
-    /// PooledString - This is the value of an entry in the pool's interning
-    /// table.
-    struct PooledString {
-      StringPool *Pool = nullptr;  ///< So the string can remove itself.
-      unsigned Refcount = 0;       ///< Number of referencing PooledStringPtrs.
+    StringPool *pool = nullptr; ///< So the string can remove itself.
+    unsigned refcount = 0;      ///< Number of referencing PooledStringPtrs.
 
-    public:
-      PooledString() = default;
-    };
 
-    friend class PooledStringPtr;
-
-    using table_t = StringMap<PooledString>;
-    using entry_t = StringMapEntry<PooledString>;
-    table_t InternTable;
+  using Entry = StringMapEntry<PooledString>;
+  StringMap<PooledString> internTable;
 
-  public:
-    StringPool();
-    ~StringPool();
 
-    /// intern - Adds a string to the pool and returns a reference-counted
-    /// pointer to it. No additional memory is allocated if the string already
-    /// exists in the pool.
-    PooledStringPtr intern(StringRef Str);
+  PooledStringPtr intern(StringRef string);
 
-    /// empty - Checks whether the pool is empty. Returns true if so.
-    ///
-    inline bool empty() const { return InternTable.empty(); }
-  };
+  bool empty() const { return internTable.empty(); }
 
-  /// PooledStringPtr - A pointer to an interned string. Use operator bool to
-  /// test whether the pointer is valid, and operator * to get the string if so.
-  /// This is a lightweight value class with storage requirements equivalent to
-  /// a single pointer, but it does have reference-counting overhead when
-  /// copied.
-  class PooledStringPtr {
-    using entry_t = StringPool::entry_t;
-
-    entry_t *S = nullptr;
+  using Entry = StringPool::Entry;
+  Entry *entry = nullptr;
 
-  public:
-    PooledStringPtr() = default;
 
-    explicit PooledStringPtr(entry_t *E) : S(E) {
-      if (S) ++S->getValue().Refcount;
-    }
+  explicit PooledStringPtr(Entry *e) : entry(e) {
+    if (entry)
+      ++entry->getValue().refcount;
 
-    PooledStringPtr(const PooledStringPtr &That) : S(That.S) {
-      if (S) ++S->getValue().Refcount;
-    }
+  PooledStringPtr(const PooledStringPtr &that) : entry(that.entry) {
+    if (entry)
+      ++entry->getValue().refcount;
 
-    PooledStringPtr &operator=(const PooledStringPtr &That) {
-      if (S != That.S) {
-        clear();
-        S = That.S;
-        if (S) ++S->getValue().Refcount;
-      }
-      return *this;
-    }
+  PooledStringPtr &operator=(const PooledStringPtr &that) {
+    if (entry != that.entry) {
+      entry = that.entry;
+      if (entry)
+        ++entry->getValue().refcount;
 
-    void clear() {
-      if (!S)
-        return;
-      if (--S->getValue().Refcount == 0) {
-        S->getValue().Pool->InternTable.remove(S);
-        S->Destroy();
-      }
-      S = nullptr;
-    }
+    if (!entry)
+    if (--entry->getValue().refcount == 0) {
+      entry->getValue().pool->internTable.remove(entry);
+      MallocAllocator allocator;
+      entry->Destroy(allocator);
+    entry = nullptr;
 
-    ~PooledStringPtr() { clear(); }
 
-    inline const char *begin() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyData();
-    }
+  const char *begin() const {
+    return entry->getKeyData();
 
-    inline const char *end() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyData() + S->getKeyLength();
-    }
+  const char *end() const {
+    return entry->getKeyData() + entry->getKeyLength();
 
-    inline unsigned size() const {
-      assert(*this && "Attempt to dereference empty PooledStringPtr!");
-      return S->getKeyLength();
-    }
+  unsigned size() const {
+    return entry->getKeyLength();
 
-    inline const char *operator*() const { return begin(); }
-    inline explicit operator bool() const { return S != nullptr; }
+  const char *operator*() const { return begin(); }
+  explicit operator bool() const { return entry != nullptr; }
 
-    inline bool operator==(const PooledStringPtr &That) const { return S == That.S; }
-    inline bool operator!=(const PooledStringPtr &That) const { return S != That.S; }
-  };
+  bool operator==(const PooledStringPtr &that) const {
+    return entry == that.entry;
+  }
+  bool operator!=(const PooledStringPtr &that) const {
+    return entry != that.entry;
+  }
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_STRINGPOOL_H

llvm/lib/IR/Value.cpp

#include "llvm/IR/Instruction.def"
 
   default:
     llvm_unreachable("attempting to delete unknown value kind");
   }
 }
 
 void Value::destroyValueName() {
   ValueName *Name = getValueName();
-  if (Name)
-    Name->Destroy();
+  if (Name) {
+    MallocAllocator Allocator;
+    Name->Destroy(Allocator);
+  }
   setValueName(nullptr);
 }
 
 bool Value::hasNUses(unsigned N) const {
   return hasNItems(use_begin(), use_end(), N);
 }
 
 bool Value::hasNUsesOrMore(unsigned N) const {
if (NameRef.empty()) {
       return;
     }
 
     // NOTE: Could optimize for the case the name is shrinking to not deallocate
     // then reallocated.
     destroyValueName();
 
     // Create the new name.
-    setValueName(ValueName::Create(NameRef));
+    MallocAllocator Allocator;
+    setValueName(ValueName::Create(NameRef, Allocator));
     getValueName()->setValue(this);
     return;
   }
 
   // NOTE: Could optimize for the case the name is shrinking to not deallocate
   // then reallocated.
   if (hasName()) {
     // Remove old name.

llvm/lib/IR/ValueSymbolTable.cpp

 #include <utility>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "valuesymtab"
 
 // Class destructor
 ValueSymbolTable::~ValueSymbolTable() {
-#ifndef NDEBUG   // Only do this in -g mode...
+#ifndef NDEBUG // Only do this in -g mode...
   for (const auto &VI : vmap)
     dbgs() << "Value still in symbol table! Type = '"
            << *VI.getValue()->getType() << "' Name = '" << VI.getKeyData()
            << "'\n";
   assert(vmap.empty() && "Values remain in symbol table!");
 #endif
 }
 
while (true) {
     auto IterBool = vmap.insert(std::make_pair(UniqueName, V));
     if (IterBool.second)
       return &*IterBool.first;
   }
 }
 
 // Insert a value into the symbol table with the specified name...
 //
-void ValueSymbolTable::reinsertValue(Value* V) {
+void ValueSymbolTable::reinsertValue(Value *V) {
   assert(V->hasName() && "Can't insert nameless Value into symbol table");
 
   // Try inserting the name, assuming it won't conflict.
   if (vmap.insert(V->getValueName())) {
     // LLVM_DEBUG(dbgs() << " Inserted value: " << V->getValueName() << ": " <<
     // *V << "\n");
     return;
   }
 
   // Otherwise, there is a naming conflict.  Rename this value.
   SmallString<256> UniqueName(V->getName().begin(), V->getName().end());
 
   // The name is too already used, just free it so we can allocate a new name.
-  V->getValueName()->Destroy();
+  MallocAllocator Allocator;
+  V->getValueName()->Destroy(Allocator);
 
   ValueName *VN = makeUniqueName(V, UniqueName);
   V->setValueName(VN);
 }
 
 void ValueSymbolTable::removeValueName(ValueName *V) {
   // LLVM_DEBUG(dbgs() << " Removing Value: " << V->getKeyData() << "\n");
   // Remove the value from the symbol table.
ValueName *ValueSymbolTable::createValueName(StringRef Name, Value *V) {
   SmallString<256> UniqueName(Name.begin(), Name.end());
   return makeUniqueName(V, UniqueName);
 }
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 // dump - print out the symbol table
 //
 LLVM_DUMP_METHOD void ValueSymbolTable::dump() const {
-  //dbgs() << "ValueSymbolTable:\n";
+  // dbgs() << "ValueSymbolTable:\n";
   for (const auto &I : *this) {
-    //dbgs() << "  '" << I->getKeyData() << "' = ";
+    // dbgs() << "  '" << I->getKeyData() << "' = ";
     I.getValue()->dump();
-    //dbgs() << "\n";
+    // dbgs() << "\n";
   }
 }
 #endif

llvm/lib/Support/StringPool.cpp

-//===-- StringPool.cpp - Interned string pool -----------------------------===//
+//===-- StringPool.cpp - Intern'd string pool -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the StringPool class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/StringPool.h"
-#include "llvm/ADT/StringRef.h"
-#include <cassert>
-
 using namespace llvm;
 
 StringPool::StringPool() {}
 
 StringPool::~StringPool() {
-  assert(InternTable.empty() && "PooledStringPtr leaked!");
+  assert(internTable.empty() && "PooledStringPtr leaked!");
 }
 
-PooledStringPtr StringPool::intern(StringRef Key) {
-  table_t::iterator I = InternTable.find(Key);
-  if (I != InternTable.end())
-    return PooledStringPtr(&*I);
-
-  entry_t *S = entry_t::Create(Key);
-  S->getValue().Pool = this;
-  InternTable.insert(S);
+PooledStringPtr StringPool::intern(StringRef key) {
+  auto it = internTable.find(key);
+  if (it != internTable.end())
+    return PooledStringPtr(&*it);
+
+  MallocAllocator allocator;
+  auto *entry = Entry::Create(key, allocator);
+  entry->getValue().pool = this;
+  internTable.insert(entry);
 
-  return PooledStringPtr(S);
+  return PooledStringPtr(entry);
 }

llvm/unittests/ADT/StringMapTest.cpp

TEST_F(StringMapTest, IterationTest) {
   // Ensure every number was visited.
   for (int i = 0; i < 100; ++i) {
     ASSERT_TRUE(visited[i]) << "Entry #" << i << " was never visited";
   }
 }
 
 // Test StringMapEntry::Create() method.
 TEST_F(StringMapTest, StringMapEntryTest) {
+  MallocAllocator Allocator;
   StringMap<uint32_t>::value_type* entry =
       StringMap<uint32_t>::value_type::Create(
-          StringRef(testKeyFirst, testKeyLength), 1u);
+          StringRef(testKeyFirst, testKeyLength), Allocator, 1u);
   EXPECT_STREQ(testKey, entry->first().data());
   EXPECT_EQ(1u, entry->second);
   free(entry);
 }
 
 // Test insert() method.
 TEST_F(StringMapTest, InsertTest) {
   SCOPED_TRACE("InsertTest");
private:
   MoveOnly(const MoveOnly &) = delete;
   MoveOnly &operator=(const MoveOnly &) = delete;
 };
 
 TEST_F(StringMapTest, MoveOnly) {
   StringMap<MoveOnly> t;
   t.insert(std::make_pair("Test", MoveOnly(42)));
   StringRef Key = "Test";
-  StringMapEntry<MoveOnly>::Create(Key, MoveOnly(42))
-      ->Destroy();
+  StringMapEntry<MoveOnly>::Create(Key, t.getAllocator(), MoveOnly(42))
+      ->Destroy(t.getAllocator());
 }
 
 TEST_F(StringMapTest, CtorArg) {
   StringRef Key = "Test";
-  StringMapEntry<MoveOnly>::Create(Key, Immovable())
-      ->Destroy();
+  MallocAllocator Allocator;
+  StringMapEntry<MoveOnly>::Create(Key, Allocator, Immovable())
+      ->Destroy(Allocator);
 }
 
 TEST_F(StringMapTest, MoveConstruct) {
   StringMap<int> A;
   A["x"] = 42;
   StringMap<int> B = std::move(A);
   ASSERT_EQ(A.size(), 0u);
   ASSERT_EQ(B.size(), 1u);

llvm/unittests/ADT/StringSetTest.cpp

-//===- llvm/unittest/ADT/StringSetTest.cpp - StringSet unit tests ----------===//
+//===- llvm/unittest/ADT/StringSetTest.cpp - StringSet unit tests ---------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringSet.h"
TEST_F(StringSetTest, IterSetKeys) {
   SmallVector<StringRef, 4> Expected = {"A", "B", "C", "D"};
   EXPECT_EQ(Expected, Keys);
 }
 
 TEST_F(StringSetTest, InsertAndCountStringMapEntry) {
   // Test insert(StringMapEntry) and count(StringMapEntry)
   // which are required for set_difference(StringSet, StringSet).
   StringSet<> Set;
-  StringMapEntry<StringRef> *Element = StringMapEntry<StringRef>::Create("A");
+  StringMapEntry<StringRef> *Element =
+      StringMapEntry<StringRef>::Create("A", Set.getAllocator());
   Set.insert(*Element);
   size_t Count = Set.count(*Element);
   size_t Expected = 1;
   EXPECT_EQ(Expected, Count);
-  Element->Destroy();
+  Element->Destroy(Set.getAllocator());
 }
 
 TEST_F(StringSetTest, EmptyString) {
   // Verify that the empty string can by successfully inserted
   StringSet<> Set;
   size_t Count = Set.count("");
   EXPECT_EQ(Count, 0UL);
 
   Set.insert("");
   Count = Set.count("");
   EXPECT_EQ(Count, 1UL);
 }
 
 } // end anonymous namespace