[libcxx-commits] [PATCH] D116944: [libc++] Split a few utilities out of __threading_support

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Looks plausible to me, FWIW, but nits.



================
Comment at: libcxx/include/__thread/convert_to_timespec.h:27
+template <class _TimeSpec>
+_LIBCPP_HIDE_FROM_ABI
+_TimeSpec __convert_to_timespec(const chrono::nanoseconds& __ns)
----------------
In making this a template, you drive-by removed the `inline` keyword. I thought we weren't doing that. :)
I have a hunch that this helper belongs in `__chrono/` not `__thread/`, but that's not a blocker.


================
Comment at: libcxx/include/module.modulemap:888-890
+      module convert_to_timespec  { private header "__thread/convert_to_timespec.h" }
+      module poll_with_backoff    { private header "__thread/poll_with_backoff.h" }
+      module timed_backoff_policy { private header "__thread/timed_backoff_policy.h" }
----------------
You aren't including any of these detail headers from the actual `<thread>`. I think you should (or else, if `<thread>` doesn't actually use them, then they're in the wrong detail directory).


================
Comment at: libcxx/src/atomic.cpp:12
 
-#include <climits>
+#include <__thread/timed_backoff_policy.h>
 #include <atomic>
----------------
I'd prefer to just include the public `<thread>`. I don't know if the Modules build will actually complain about including a private header like this from `src/`, but I just think it's better hygiene if `src/` uses only the public headers.

(Pre-existing: At some point I'd love to find out why some `src/*.cpp` do e.g. `#include "memory"` instead of `#include <memory>`, and whether it'd be safe to change that.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116944