[PATCH] D133713: [Support] Introduce ThreadSafeAllocator

[David Blaikie via Phabricator via llvm-commits]

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

@benlangmuir did you have outstanding feedback on this patch? Otherwise it's fine by me, I think.



================
Comment at: llvm/include/llvm/Support/ThreadSafeAllocator.h:38-46
+  auto Allocate(size_t N) {
+    LockGuard Lock(Flag);
+    return Alloc.Allocate(N);
+  }
+
+  auto Allocate(size_t Size, size_t Align) {
+    LockGuard Lock(Flag);
----------------
Might be nice to implement these with `applyLocked` - to keep the locking in one place - not that three short places close together are a real problem


================
Comment at: llvm/include/llvm/Support/ThreadSafeAllocator.h:48-51
+  void applyLocked(llvm::function_ref<void(AllocatorType &Alloc)> Fn) {
+    LockGuard Lock(Flag);
+    Fn(Alloc);
+  }
----------------
If this is in a template/header anyway, and pretty short, maybe make it a template that takes a functor, rather than adding the overhead of `llvm::function_ref`s type erasure?


================
Comment at: llvm/unittests/Support/ThreadSafeAllocatorTest.cpp:38
+  }
+  void endBusy() {
+    {
----------------
This name's a bit confusing to me - as this doesn't test or set the `IsBusy` flag, but has `Busy` in the name.

Make slightly longer or more descriptive names would make it clear what each of these member functions is for/does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133713