[Mlir-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)

[Georgios Pinitas]

================
@@ -227,7 +265,7 @@ class ThreadPool {
 class ThreadPoolTaskGroup {
----------------
GeorgeARM wrote:

So the way I see this is:

I have a public API that looks like:
```
  virtual void asyncEnqueue(std::function<void()> Task, ThreadPoolTaskGroup *Group) = 0;
 // or similar
```

My thoughts:
* `ThreadPoolTaskGroup` implementation is actually internal to the `ThreadPool`
* `ThreadPoolTaskGroup` being passed to the API but only used internally as just a UID. No member functions being called internally. Moreover, the object definition itself is a RAII wrapper on top of the ThreadPool API implementation so I find that this is an unnecessary circular dependency and coupling.
* Uniqueness is `guaranteed` through memory addressing. Someone can `break` uniqueness when going through the public `ThreadPool` API.
* Type safety; sure you don't have implicit conversions going on.

What `guarantees` to you uniqueness is essentially when you use RAII wrapper itself and the fact that calls at termination: ` ~ThreadPoolTaskGroup() { wait(); }` which will go through; process the tasks and erase the group from the `ActiveGroups`.

So what I think is that passing instead of passing `*this` you can convert the address to a string/int and pass this instead.

Eitherway, please ignore my comment if it doesn't make sense; as said not familiar with the codebase. I just find strange a bit this aspect of the API.


https://github.com/llvm/llvm-project/pull/82094