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

[Dmitry via Phabricator via llvm-commits]

Dmitry.Kozhevnikov added inline comments.


================
Comment at: include/llvm/Support/Threading.h:53
 
 /// llvm_execute_on_thread - Execute the given \p UserFn on a separate
 /// thread, passing it the provided \p UserData and waits for thread
----------------
jfb wrote:
> Can you remove the `llvm_execute_on_thread` while you're here?
Should I? It's used twice: at `CrashRecoveryContext::RunSafelyOnThread`, and, which is probably worse, exposed via the libclang (tools/clang/include/clang-c/Index.h, `clang_executeOnThread`). My understanding was this API is meant to be stable? That was one of the reasons I haven't touched its interface at all.

Also, while looking for this, I've found the fallback implementation of `llvm_execute_on_thread`, in `lib/Support/Threading.cpp`, which just runs the callback synchronously. Sorry, I haven't updated its signature, will do. I've probably should also add the new function there, that would just raise a fatal error since it's meaningless to call it with multithreading disabled.


================
Comment at: lib/Support/Threading.cpp:122
+                              JoiningPolicy::Detach);
+  Info.release();
+}
----------------
jfb wrote:
> I'd replace `get` above with `release`, and not have the `release` here, since you're kinda "moving" ownership into a `void*`.
I think in the current state, I can just drop `unique_ptr` and just pass a raw pointer returned by `new` directly.


================
Comment at: lib/Support/Unix/Threading.inc:65
+  if (::pthread_attr_init(&Attr) != 0) {
+    ReportErrnoFatal("pthread_attr_init failed");
+  }
----------------
jfb wrote:
> Here and below, pthread doesn't set errno: it returns the error directly. You should pass the result in and call `strerror`.
Sorry, will do.


https://reviews.llvm.org/D51103