[libc++] Granularize chrono includes

[• Quuxplusone]

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

[Mordante]

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

[• Quuxplusone]

libcxx/include/future

378

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?

[philnik]

• Address comments

[Mordante]

libcxx/include/future

378

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.

[• Quuxplusone]

libcxx/include/future

378

Sorry, I meant "include"

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

[• Quuxplusone]

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

[EricWF]

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?

[• Quuxplusone]

In D120141#3341656, @EricWF wrote:

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.

libcxx/docs/ReleaseNotes.rst

 - The ``_LIBCPP_ABI_UNSTABLE`` macro has been removed in favour of setting
   ``_LIBCPP_ABI_VERSION=2``. This should not have any impact on users because
   they were not supposed to set ``_LIBCPP_ABI_UNSTABLE`` manually, however we
   still feel that it is worth mentioning in the release notes in case some users
   had been doing it.
 - The header ``<experimental/filesystem>`` has been removed. Instead, use
   ``<filesystem>`` header. The associated macro
   ``_LIBCPP_DEPRECATED_EXPERIMENTAL_FILESYSTEM`` has also been removed.
 
-- Transitive includes of ``<algorithm>`` have been removed. If you see compiler errors
-  related to missing declarations inside namespace ``std`` when updating libc++,
-  you are probably missing ``#include <algorithm>`` in a file where you use algorithms.
-
+- 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>``.
 ABI Changes
 -----------
 
 - The ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` macro controlling whether we use an
   emulation for ``std::nullptr_t`` in C++03 mode has been removed. After this change,
   ``_LIBCPP_ABI_USE_CXX03_NULLPTR_EMULATION`` will not be honoured anymore and there
   will be no way to opt back into the C++03 emulation of ``std::nullptr_t``.
 
 Build System Changes
 --------------------
 
 - Support for standalone builds have been entirely removed from libc++, libc++abi and
   libunwind. Please use :ref:`these instructions <build instructions>` for building
   libc++, libc++abi and/or libunwind.

libcxx/include/__filesystem/directory_entry.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___FILESYSTEM_DIRECTORY_ENTRY_H
 #define _LIBCPP___FILESYSTEM_DIRECTORY_ENTRY_H
 
 #include <__availability>
+#include <__chrono/time_point.h>
 #include <__config>
 #include <__errc>
 #include <__filesystem/file_status.h>
 #include <__filesystem/file_time_type.h>
 #include <__filesystem/file_type.h>
 #include <__filesystem/filesystem_error.h>
 #include <__filesystem/operations.h>
 #include <__filesystem/path.h>
 #include <__filesystem/perms.h>
 #include <__utility/unreachable.h>
-#include <chrono>
 #include <cstdint>
 #include <cstdlib>
 #include <iosfwd>
 #include <system_error>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif

libcxx/include/__filesystem/file_time_type.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___FILESYSTEM_FILE_TIME_TYPE_H
 #define _LIBCPP___FILESYSTEM_FILE_TIME_TYPE_H
 
 #include <__availability>
+#include <__chrono/file_clock.h>
+#include <__chrono/time_point.h>
 #include <__config>
-#include <chrono>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifndef _LIBCPP_CXX03_LANG
 
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
 
 typedef chrono::time_point<_FilesystemClock> file_time_type;
 
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 
 #endif // _LIBCPP_CXX03_LANG
 
 #endif // _LIBCPP___FILESYSTEM_FILE_TIME_TYPE_H

libcxx/include/__filesystem/operations.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___FILESYSTEM_OPERATIONS_H
 #define _LIBCPP___FILESYSTEM_OPERATIONS_H
 
 #include <__availability>
+#include <__chrono/time_point.h>
 #include <__config>
 #include <__filesystem/copy_options.h>
 #include <__filesystem/file_status.h>
 #include <__filesystem/file_time_type.h>
 #include <__filesystem/file_type.h>
 #include <__filesystem/path.h>
 #include <__filesystem/perm_options.h>
 #include <__filesystem/perms.h>
 #include <__filesystem/space_info.h>
-#include <chrono>
 #include <cstdint>
 #include <system_error>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifndef _LIBCPP_CXX03_LANG

libcxx/include/__mutex_base

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___MUTEX_BASE
 #define _LIBCPP___MUTEX_BASE
 
+#include <__chrono/duration.h>
+#include <__chrono/steady_clock.h>
+#include <__chrono/system_clock.h>
+#include <__chrono/time_point.h>
 #include <__config>
 #include <__threading_support>
-#include <chrono>
 #include <ratio>
 #include <system_error>
 #include <time.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 

libcxx/include/__thread/poll_with_backoff.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef _LIBCPP___THREAD_POLL_WITH_BACKOFF_H
 #define _LIBCPP___THREAD_POLL_WITH_BACKOFF_H
 
 #include <__availability>
+#include <__chrono/duration.h>
+#include <__chrono/high_resolution_clock.h>
+#include <__chrono/steady_clock.h>
+#include <__chrono/time_point.h>
 #include <__config>
-#include <chrono>
+#include <__filesystem/file_time_type.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 static _LIBCPP_CONSTEXPR const int __libcpp_polling_count = 64;

libcxx/include/__thread/timed_backoff_policy.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef _LIBCPP___THREAD_TIMED_BACKOFF_POLICY_H
 #define _LIBCPP___THREAD_TIMED_BACKOFF_POLICY_H
 
 #include <__config>
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 
-#include <__threading_support>
-#include <chrono>
+#  include <__chrono/duration.h>
+#  include <__threading_support>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 struct __libcpp_timed_backoff_policy {

libcxx/include/__threading_support

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP_THREADING_SUPPORT
 #define _LIBCPP_THREADING_SUPPORT
 
 #include <__availability>
+#include <__chrono/convert_to_timespec.h>
+#include <__chrono/duration.h>
 #include <__config>
 #include <__thread/poll_with_backoff.h>
-#include <chrono>
 #include <errno.h>
 #include <iosfwd>
 #include <limits>
 
 #ifdef __MVS__
 # include <__support/ibm/nanosleep.h>
 #endif
 

libcxx/include/atomic

 
 #define ATOMIC_FLAG_INIT see below
 
 }  // std
 
 */
 
 #include <__availability>
+#include <__chrono/duration.h>
 #include <__config>
 #include <__thread/poll_with_backoff.h>
 #include <__thread/timed_backoff_policy.h>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
 #include <type_traits>
 #include <version>

libcxx/include/future

 template <class R, class Alloc> struct uses_allocator<packaged_task<R>, Alloc>;
 
 }  // std
 
 */
 
 #include <__assert>
 #include <__availability>
+#include <__chrono/duration.h>
+#include <__chrono/time_point.h>
 #include <__config>
 #include <__memory/allocator_arg_t.h>
 #include <__memory/uses_allocator.h>
 #include <__utility/auto_cast.h>
 #include <__utility/forward.h>
-#include <chrono>
 #include <exception>
 #include <memory>
 #include <mutex>
 #include <system_error>
 #include <thread>
 #include <version>
 
+#include <chrono> // TODO: Remove unused header
+
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #ifdef _LIBCPP_HAS_NO_THREADS
 #error <future> is not supported on this single threaded system
 #else // !_LIBCPP_HAS_NO_THREADS
 

libcxx/include/semaphore

 
 using binary_semaphore = counting_semaphore<1>;
 
 }
 
 */
 
 #include <__availability>
+#include <__chrono/time_point.h>
 #include <__config>
 #include <__thread/timed_backoff_policy.h>
 #include <__threading_support>
 #include <atomic>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header

libcxx/include/thread

 
 #include <__assert>
 #include <__config>
 #include <__mutex_base>
 #include <__thread/poll_with_backoff.h>
 #include <__thread/timed_backoff_policy.h>
 #include <__threading_support>
 #include <__utility/forward.h>
-#include <chrono>
 #include <cstddef>
 #include <functional>
 #include <iosfwd>
 #include <memory>
 #include <system_error>
 #include <tuple>
 #include <type_traits>
 #include <version>

libcxx/test/std/thread/futures/futures.async/async.pass.cpp

 //     future<typename result_of<F(Args...)>::type>
 //     async(F&& f, Args&&... args);
 
 // template <class F, class... Args>
 //     future<typename result_of<F(Args...)>::type>
 //     async(launch policy, F&& f, Args&&... args);
 
 
-#include <future>
 #include <atomic>
-#include <memory>
 #include <cassert>
+#include <chrono>
+#include <future>
+#include <memory>
 
 #include "test_macros.h"
 
 typedef std::chrono::high_resolution_clock Clock;
 typedef std::chrono::milliseconds ms;
 
 std::atomic_bool invoked{false};
 

libcxx/test/std/thread/futures/futures.shared_future/wait.pass.cpp

 // UNSUPPORTED: c++03
 
 // <future>
 
 // class shared_future<R>
 
 // void wait() const;
 
-#include <future>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 void func1(std::promise<int> p)
 {
     std::this_thread::sleep_for(std::chrono::milliseconds(500));
     p.set_value(3);

libcxx/test/std/thread/futures/futures.shared_future/wait_for.pass.cpp

 // <future>
 
 // class shared_future<R>
 
 // template <class Rep, class Period>
 //   future_status
 //   wait_for(const chrono::duration<Rep, Period>& rel_time) const;
 
-#include <future>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 typedef std::chrono::milliseconds ms;
 
 static const ms sleepTime(500);
 static const ms waitTime(5000);

libcxx/test/std/thread/futures/futures.shared_future/wait_until.pass.cpp

 // <future>
 
 // class shared_future<R>
 
 // template <class Clock, class Duration>
 //   future_status
 //   wait_until(const chrono::time_point<Clock, Duration>& abs_time) const;
 
-#include <future>
 #include <atomic>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 enum class WorkerThreadState { Uninitialized, AllowedToRun, Exiting };
 typedef std::chrono::milliseconds ms;
 
 std::atomic<WorkerThreadState> thread_state(WorkerThreadState::Uninitialized);

libcxx/test/std/thread/futures/futures.unique_future/wait.pass.cpp

 // UNSUPPORTED: c++03
 
 // <future>
 
 // class future<R>
 
 // void wait() const;
 
-#include <future>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 void func1(std::promise<int> p)
 {
     std::this_thread::sleep_for(std::chrono::milliseconds(500));
     p.set_value(3);

libcxx/test/std/thread/futures/futures.unique_future/wait_for.pass.cpp

 // <future>
 
 // class future<R>
 
 // template <class Rep, class Period>
 //   future_status
 //   wait_for(const chrono::duration<Rep, Period>& rel_time) const;
 
-#include <future>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 typedef std::chrono::milliseconds ms;
 
 static const ms sleepTime(500);
 static const ms waitTime(5000);

libcxx/test/std/thread/futures/futures.unique_future/wait_until.pass.cpp

 // <future>
 
 // class future<R>
 
 // template <class Clock, class Duration>
 //   future_status
 //   wait_until(const chrono::time_point<Clock, Duration>& abs_time) const;
 
-#include <future>
 #include <atomic>
 #include <cassert>
+#include <chrono>
+#include <future>
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 enum class WorkerThreadState { Uninitialized, AllowedToRun, Exiting };
 typedef std::chrono::milliseconds ms;
 
 std::atomic<WorkerThreadState> thread_state(WorkerThreadState::Uninitialized);