[PATCH] D51103: [Support] Add a way to run a function on a detached thread

[JF Bastien via Phabricator via llvm-commits]

jfb requested changes to this revision.
jfb added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Support/Threading.h:81
+void llvm_execute_on_thread_async(llvm::unique_function<void()> Func,
+                                  unsigned RequestedStackSize = 0);
+
----------------
Seems like you want an optional<> for stack size. Can you also change the name so it contains the unit? i.e. Requested StackSizeInBytes.


================
Comment at: lib/Support/Unix/Threading.inc:59
+                                        unsigned RequestedStackSize,
+                                        bool ShouldJoin) {
   // Construct the attributes object.
----------------
Please change ShouldJoin to an `enum class` instead of a `bool`. Ditto on the return value.


================
Comment at: lib/Support/Unix/Threading.inc:77
+  if (::pthread_create(&Thread, &Attr, ThreadFunc, Arg) != 0)
+    return false;
 
----------------
So any failure to init attr, set stack size, or create the thread just... silently fail? I know this is pre-existing, but that's terrible! Is there ever a situation when we want to execute something on a thread, or just not execute it at all? It seems like we should abort if any of these fail?


https://reviews.llvm.org/D51103