labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
Although it may not seem from the number of my comments, I actually quite like
this implementation. However, I think more work needs to be done to make this
future-proof.
- we definitely need unit tests for this.
- we need more documentation (especially on the public interfaces, but the
trickier internal parts could use some explanation as well).
================
Comment at: include/lldb/Utility/TaskPool.h:23
@@ +22,3 @@
+
+ // Add a new task to the thread pool and return a std::future belongs for
the newly created task.
+ // The caller of this function have to wait on the future for this task to
complete.
----------------
belonging to
================
Comment at: include/lldb/Utility/TaskPool.h:24
@@ +23,3 @@
+ // Add a new task to the thread pool and return a std::future belongs for
the newly created task.
+ // The caller of this function have to wait on the future for this task to
complete.
+ template<typename F, typename... Args>
----------------
s/have/has
================
Comment at: include/lldb/Utility/TaskPool.h:33
@@ +32,3 @@
+ static void
+ RunTasks(T&&... t);
+
----------------
Perhahs the entire TaskPool could be an implementation detail and this
functionality would be accessed (via a static method on) TaskRunner? What do
you think? I have found it confusing it the other commit, why you sometimes use
TaskPool and another time TaskRunner...
================
Comment at: include/lldb/Utility/TaskPool.h:80
@@ +79,3 @@
+ void
+ WaitForAllTask();
+
----------------
WaitForAllTasks
================
Comment at: include/lldb/Utility/TaskPool.h:103
@@ +102,3 @@
+
+template<typename H, typename... T>
+struct TaskPool::RunTaskImpl<H, T...>
----------------
Suggestion: name these `Head`, `Tail`. You use `T` for other purposes
elsewhere, and I have spent a lot of time trying to figure out why is this `H`
and not `F` before I figured out what's going on here.
================
Comment at: include/lldb/Utility/TaskPool.h:126
@@ +125,3 @@
+{
+ auto task = std::make_shared<std::packaged_task<typename
std::result_of<F(Args...)>::type()>>(
+ std::bind(std::forward<F>(f), std::forward<Args>(args)...));
----------------
task_sp
================
Comment at: include/lldb/Utility/TaskPool.h:138
@@ +137,3 @@
+
+template <typename T>
+template<typename F, typename... Args>
----------------
You use `T` for both "task" and the "result value of a task". Could you
disambiguate the two. Suggestion: `Ret` (or `Return`) and `Task`.
================
Comment at: include/lldb/Utility/TaskPool.h:145
@@ +144,3 @@
+ auto it = m_pending.emplace(m_pending.end());
+ *it = std::move(TaskPool::AddTask(
+ [this, it](F&& f, Args&&... args)
----------------
This construction is quite convoluted. Do you actually need a list of the
pending tasks anywhere? As far as I can tell, you only need to check if there
are any tasks pending, which can be done with a simple (atomic) integer.
================
Comment at: include/lldb/Utility/TaskPool.h:148
@@ +147,3 @@
+ {
+ T&& r = f(args...);
+
----------------
Is `std::forward` needed here?
http://reviews.llvm.org/D13727
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
