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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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)
----------------
Quuxplusone wrote:
> 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.
> In making this a template, you drive-by removed the `inline` keyword. I thought we weren't doing that. :)

You're right -- I removed it because templates implicitly have the right linkage, but I forgot about the fact that it can mess up optimizations (and I'm still working on figuring out the regression I mentioned elsewhere). I'll put `inline` back, good catch.

> I have a hunch that this helper belongs in `__chrono/` not `__thread/`, but that's not a blocker.

Yeah, I guess you're right, I'll move it there.



================
Comment at: libcxx/src/atomic.cpp:12
 
-#include <climits>
+#include <__thread/timed_backoff_policy.h>
 #include <atomic>
----------------
Quuxplusone wrote:
> 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.)
> I'd prefer to just include the public `<thread>`.

Done.

> (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.)

I'm pretty sure it's safe to change that, it's just historical inconsistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116944