[libcxx-commits] [PATCH] D120141: [libc++] Granularize chrono includes

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general looks good, but please double check whether all public parts have the proper includes.



================
Comment at: libcxx/include/__thread/timed_backoff_policy.h:12
 
+#include <__chrono/duration.h>
 #include <__config>
----------------
Can you move this to the original location so it's guarded by `#ifndef _LIBCPP_HAS_NO_THREADS`?


================
Comment at: libcxx/include/future:376
 #include <thread>
 #include <version>
 
----------------
Quuxplusone wrote:
> `future` must need `time_point` and `duration` for `future::wait_{for,until}`, right? I would hope that the CI Modules build catches this... right?
> 
> ...However, looking at the changes you had to make to the unit tests, I think we definitely need to Hyrum's-Law this. Please add whatever `__chrono/*` headers are needed here, but then also `#include <chrono> // TODO: remove this`, and then remove that line in a separate PR. (I'll approve that PR unless someone tells me not to, since AFAIK we've agreed that this month is the right time to break Hyrum's Law because it's early in the LLVM 15 release cycle.)
Yes this was the time to do this. Please update the release notes with this header removal.

But when parts of chrono are part of the public interface, the let's make sure we export the required (sub)header.

The modular build doesn't catch everything since we use `export *` in our modules. I don't recall exactly why we can't remove that line, but I assume we need to remove that at some point when we want to support real module builds. (I'm not sure how complete module support in Clang is.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120141