Details
- Reviewers
Mordante • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG489637e66dd3: [libc++] Granularize chrono includes
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM except for the Hyrum's Law / IWYU comment in <future>. I'll let someone else be the second libc++ reviewer, though, because more eyeballs will be good.
| libcxx/include/future | ||
|---|---|---|
| 378 | 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.) | |
In general looks good, but please double check whether all public parts have the proper includes.
| libcxx/include/__thread/timed_backoff_policy.h | ||
|---|---|---|
| 12 | Can you move this to the original location so it's guarded by #ifndef _LIBCPP_HAS_NO_THREADS? | |
| libcxx/include/future | ||
| 378 | 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.) | |
| libcxx/include/future | ||
|---|---|---|
| 378 |
@Mordante Can you clarify and/or give an example of what you mean here? FWIW, I have a prejudice against complicating the modulemap (that was one of the reasons for removing __function_like from the niebloids, although ultimately not the deciding factor). If this is what you mean: I don't (yet) see a reason that e.g. <future>'s exported declaration of std::future_status wait_for(const std::chrono::duration<Rep,Period>&) const should also require <future> to export a declaration of time_point. Anyone using that method must be calling it like myFuture.wait_until(std::chrono::seconds(42)), which means they themselves must already have access to <chrono>. I've run into adjacent situations in things like D89057 std::pmr::vector, where someone might #include <vector> and then try to define a std::pmr::vector<int> v; which requires that a definition of std::pmr::polymorphic_allocator<int> actually be reachable at that point (because we need to know its sizeof). But even in D89057 I don't think that #include <vector> needs to permit the user to name std::pmr::polymorphic_allocator; and even more so, I don't currently see why #include <future> should permit the user to name std::chrono::seconds. Do you see something I don't? (IOW, what's a minimal example translation unit you want to (still) be compilable after this PR, and what change would you make to this PR to make the example compilable? | |
| libcxx/include/future | ||
|---|---|---|
| 378 |
Sorry I meant include.
The types are used in this header, for example template <class _Rep, class _Period>
inline
future_status
__assoc_sub_state::wait_for(const chrono::duration<_Rep, _Period>& __rel_time) const
{
return wait_until(chrono::steady_clock::now() + __rel_time);
}Then I expect the header to include a header declaring chrono::duration and chrono::steady_clock. Without the declarations is can use a global version of these symbols. I feel it's more a hygiene part of the header to at least have the declarations it uses, not just to benefit the user. So including the parts of <chrono> it uses suffices for me. | |
| libcxx/include/future | ||
|---|---|---|
| 378 |
Ahhh. +1 then: include-what-you-use. :) | |
LGTM % the wording of the new release note.
| libcxx/docs/ReleaseNotes.rst | ||
|---|---|---|
| 55–60 | These two notes give conflicting advice. Solution: combine the notes! My recommendation: - Some libc++ headers no longer transitively include all of ``<algorithm>`` and ``<chrono>``. If, after updating libc++, you see compiler errors related to missing declarations in namespace ``std``, it might be because one of your source files now needs to ``#include <algorithm>`` and/or ``#include <chrono>``. (And then we should expand that same note as we remove more transitive includes.) | |
I know this has already been litigated, but I was hoping someone could fill
me in on what exactly we're trying to accomplish by breaking up these
includes?
Various partial answers depending on what exactly the question is:
- By including just the bits of <chrono> we need, instead of all of <chrono>, we (handwavily, might) speed up compile times.
- By including just the bits of <chrono> we need, we make the code a little bit more self-documenting: libc++ header H uses exactly these components from <chrono>, no need for the reader to hunt each of them down by eyeballing H's code.
- By including just the bits of <chrono> we need, we reduce the amount of std:: stuff transitively made available to users, which jibes with @ldionne's general "no extensions, nothing accidentally works here and breaks elsewhere" philosophy.
- By splitting up <chrono> (D116965), we gained some consistency, because it is (now) libc++ style that big headers get split up. Now nobody has to ask "Why is <memory> split up and <chrono> isn't?" (Nor "Why do so many libc++ headers transitively include all of <chrono>, when we don't do that with any other header?") The snowball has rolled far enough by now that consistency is the killer app for me at this point. :)
- By splitting up headers in general, we (arguably) make them easier to read and maintain. For <chrono> I think this argument is particularly good, because C++20 makes it double or triple in size. (Ditto <algorithm>, <ranges>, <iterator>, <memory>...) The argument lacks potency when applied to already-small things like <string>, <optional>, <any>, which is kinda why nobody's bothered to split those up yet.
- By splitting up headers in general, we handwavily make it easier to deal with the circular dependencies beloved by LEWG. The previous approach was to lift dependencies as needed into <__foo_base>, and then, if all else failed, into <type_traits>. FWIW I personally liked that approach just fine, but I admit it requires a high level of institutional knowledge about why certain things are in <__string> not <string>, why std::swap is in <type_traits>, and so on, which (arguably) doesn't scale if you want new contributors to make themselves useful quickly. "Pull out everything super fine-grained" is arguably lazy, but it works and it's easy to explain.
These two notes give conflicting advice. Solution: combine the notes! My recommendation:
(And then we should expand that same note as we remove more transitive includes.)