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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In D59130#1434495 <https://reviews.llvm.org/D59130#1434495>, @kadircet wrote:

> > Realistically nobody reads the comments, so I'm OK not having them. I just want to know that @kadircet read the documentation for the platform-specific APIs being introduced, and has reasonable suspicion that no gotcha is being introduced.
>
> Have you checked the latest changes in the patch? I've already added comments for those platform specific behaviors as documented in their relevant web pages(because you've requested before). And yes, I had already read the documentation beforehand, AFAICT no gotcha is being introduced.
>  Please feel free to either just read the parts of the documentation I included in comments or sources themselves in urls and notify if there are some gotchas in any platforms.


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.



================
Comment at: include/llvm/Support/Threading.h:182
+  /// Returns true on success.
+  bool set_thread_priority(ThreadPriority Priority);
 }
----------------
I'm not a fan of returning `bool`. Please return a custom `enum class`.


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


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


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59130