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

Wed Feb 23 12:15:59 PST 2022

Quuxplusone added inline comments.

================
Comment at: libcxx/include/future:376
 #include <thread>
 #include <version>

----------------
Mordante wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > 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.)
> > > But when parts of chrono are part of the public interface, the let's make sure we export the required (sub)header.
> > 
> > @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?
> > > But when parts of chrono are part of the public interface, the let's make sure we export the required (sub)header.
>  
> Sorry I meant include.
> 
> > @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?
> 
> 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. 
> 
> 
> Sorry, I meant "include"

Ahhh. +1 then: include-what-you-use. :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120141