[PATCH] D133713: [Support] Introduce ThreadSafeAllocator

[Joe Loser via Phabricator via llvm-commits]

jloser added inline comments.


================
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);
----------------
dblaikie wrote:
> 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
+1 for just delegating to `applyLocked` (if we make it a function template) rather than delegating to something that uses type erasure IMO.


================
Comment at: llvm/include/llvm/Support/ThreadSafeAllocator.h:48-51
+  void applyLocked(llvm::function_ref<void(AllocatorType &Alloc)> Fn) {
+    LockGuard Lock(Flag);
+    Fn(Alloc);
+  }
----------------
dblaikie wrote:
> 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?
+1 for making this a function template 


================
Comment at: llvm/unittests/Support/ThreadSafeAllocatorTest.cpp:38
+  }
+  void endBusy() {
+    {
----------------
dblaikie wrote:
> 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?
+1 as well re the naming/confusion to a new reader of the code (such as myself)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133713