[PATCH] D95654: ADT: Sink the guts of StringMapEntry::Create into StringMapEntryBase

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:30-34
+
+protected:
+  template <typename AllocatorTy>
+  static void *allocateWithKey(size_t EntrySize, size_t EntryAlign,
+                               StringRef Key, AllocatorTy &Allocator);
----------------
dblaikie wrote:
> Took me a minute to figure out why this was a member function - since it doesn't need access to any members, etc. Perhaps it could be a free function in a detail or implementation namespace? Not sure if we have a strong convention of how that's done in LLVM.
I have a tendency to hide things as static class members to avoid having to think of good generic names (the context of `StringMapEntryBase` makes `allocateWithKey` clear enough).

I'll think for a bit about the name for the detail/implementation namespace; feel free to recommend one; if I don't find one after a bit of thought I'll probably commit as-is.

One thought is that this could be done in a mnore generic way; MemoryBuffer.cpp has some similar code (it tail allocates the null-terminated identifier, and then also the buffer contents if owned).


================
Comment at: llvm/include/llvm/ADT/StringMapEntry.h:48-55
+  assert(Allocation && "Unhandled out-of-memory");
+
+  // Copy the string information.
+  char *Buffer = reinterpret_cast<char *>(Allocation) + EntrySize;
+  if (KeyLength > 0)
+    ::memcpy(Buffer, Key.data(), KeyLength);
+  Buffer[KeyLength] = 0; // Null terminate for convenience of clients.
----------------
dblaikie wrote:
> Guess this half could be pulled out into a non-template function, but maybe that doesn't tilt the inliner enough & hurts performance?
Good point; I bet it's worth it.

Along similar lines, we could add a MallocAllocator overload (unlike BumpPtrAllocator::Allocate, inlining MallocAllocator::Allocate doesn't seem valuable) and define it in StringMap.cpp. I *think* this wouldn't require writing the code twice, since the overload could explicitly call the template:
```
void *StringMapEntryBase::allocateWithKey(
    size_t EntrySize, size_t EntryAlign, SringRef Key,
    MallocAllocator &Allocator) {
  // Reuse the templated code but instantiate it in the .cpp to avoid code
  // duplication.
  return allocateWithKey<MallocAllocator>(
      EntrySize, EntryAlign, Key, Allocator);
}
```

I'll look at these as follow ups.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95654