[PATCH] D59130: [llvm][Support] Provide interface to set thread priorities

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added a comment.

In D59130#1435036 <https://reviews.llvm.org/D59130#1435036>, @jfb wrote:

> I had, and it doesn't really answer my questions. Maybe the documentation is lacking. I'm not sure the point I'm trying to make is getting across, so I'll set my own priority to background.


Could you help me get your point then? I would like to address everyone's concerns in such a patch, sorry for not getting it the first time :/
I thought you were asking to make sure all of the platform specific functions I've used is behaving ~same, meaning lowering a thread's priority and letting it run in a way that won't affect foreground threads, without any extra gotchas like "doing IO is forbidden". 
This was the thing I first checked while coming up with this patch already and per your request I've also put the sources I've checked into the comments with most relevant details inlined.



================
Comment at: include/llvm/Support/Threading.h:182
+  /// Returns true on success.
+  bool set_thread_priority(ThreadPriority Priority);
 }
----------------
jfb wrote:
> I'm not a fan of returning `bool`. Please return a custom `enum class`.
I don't think it is feasible for every function that wants to indicate Failure/Success to come up with its own custom enum. Going with it anyway per you request, but wondering if at least llvm::Error would be a better choice or not ?


================
Comment at: lib/Support/Unix/Threading.inc:234
+      Priority == ThreadPriority::Background ? SCHED_IDLE : SCHED_OTHER,
+      &priority);
+#elif defined(__APPLE__)
----------------
jfb wrote:
> The manpage says "On success, these functions return 0; on error, they return a nonzero error number".
hence the not ?


================
Comment at: lib/Support/Unix/Threading.inc:251
+                      Priority == ThreadPriority::Background ? PRIO_DARWIN_BG
+                                                             : 0);
+#endif
----------------
jfb wrote:
> The manpage says "The setpriority() call returns 0 if there is no error, or -1 if there is".
hence the not ?


================
Comment at: lib/Support/Unix/Threading.inc:252
+                                                             : 0);
+#endif
+}
----------------
jfb wrote:
> The function doesn't return in all cases.
Thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59130