[libc++] Remove the chrono include from algorithm

[iana]

Summary

algorithm's include of chrono causes include cycles:

algorithm -> chrono -> __chrono/convert_to_tm.h -> __chrono/statically_widen.h -> __format/concepts.h -> __format/format_parse_context.h -> string_view -> algorithm

algorithm -> chrono -> __chrono/convert_to_tm.h -> __chrono/statically_widen.h -> __format/concepts.h -> __format/format_parse_context.h -> string_view -> functional -> __functional/boyer_moore_searcher.h -> array -> algorithm

algorithm -> chrono -> __chrono/convert_to_tm.h -> __chrono/statically_widen.h -> __format/concepts.h -> __format/format_parse_context.h -> string_view -> functional -> __functional/boyer_moore_searcher.h -> unordered_map -> algorithm

algorithm -> chrono -> __chrono/convert_to_tm.h -> __chrono/statically_widen.h -> __format/concepts.h -> __format/format_parse_context.h -> string_view -> functional -> __functional/boyer_moore_searcher.h -> vector -> algorithm

This is a problem for clang modules after the std mega module is broken up, because it becomes a module cycle which is a hard error.

All of the includes in the __chrono and __format headers are being used and so can't be removed. algorithm's include of chrono is already removed in C++20, whereas the array, string_view, unordered_map, vector includes of algorithm aren't removed until C++23 (and it's 4x the includes that would need removing). Unconditionally remove the chrono include from algorithm in all versions, so that the module breakup can happen (the module has to apply to all C++ versions).

[Mordante]

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

[iana]

Remove algorithm -> chrono from the transitive include tests. Add a release note saying the include was removed and why.

[iana]

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

[Mordante]

In D148405#4271485, @iana wrote:

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

Check-cxx needs to be executed for every language version. If you intend to do more of these cleanups I can explain what to do. If it's only one it's easier for me to commandeer the revision and apply the fixes.

libcxx/docs/ReleaseNotes.rst

70–71

In the release notes we usually give no rationale.

[iana]

Remove the rationale for removing algorithm -> chrono from the release notes.

[iana]

In D148405#4274218, @Mordante wrote:

In D148405#4271485, @iana wrote:

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

Check-cxx needs to be executed for every language version. If you intend to do more of these cleanups I can explain what to do. If it's only one it's easier for me to commandeer the revision and apply the fixes.

Ah. This was the only pure include cycle I found, but it's possible there will be some module cycles that still need to be broken up. It'd be good for me to know how to run it properly anyway, but also feel free to commandeer if you prefer.

[Mordante]

In D148405#4274364, @iana wrote:

In D148405#4274218, @Mordante wrote:

In D148405#4271485, @iana wrote:

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

Check-cxx needs to be executed for every language version. If you intend to do more of these cleanups I can explain what to do. If it's only one it's easier for me to commandeer the revision and apply the fixes.

Ah. This was the only pure include cycle I found, but it's possible there will be some module cycles that still need to be broken up. It'd be good for me to know how to run it properly anyway, but also feel free to commandeer if you prefer.

Actually the method is described in libcxx/test/libcxx/transitive_includes.sh.cpp:45 re-generating the files is done by building the target libcxx-generate-files.

[iana]

Fix a test failure

[iana]

Fix another test failure.

[iana]

Fixed more tests

[iana]

In D148405#4277800, @Mordante wrote:

In D148405#4274364, @iana wrote:

In D148405#4274218, @Mordante wrote:

In D148405#4271485, @iana wrote:

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

Check-cxx needs to be executed for every language version. If you intend to do more of these cleanups I can explain what to do. If it's only one it's easier for me to commandeer the revision and apply the fixes.

Ah. This was the only pure include cycle I found, but it's possible there will be some module cycles that still need to be broken up. It'd be good for me to know how to run it properly anyway, but also feel free to commandeer if you prefer.

Actually the method is described in libcxx/test/libcxx/transitive_includes.sh.cpp:45 re-generating the files is done by building the target libcxx-generate-files.

Thanks, I think I got the tests all squared away. I'm not sure what's up with the sanitizer and aix failures, but they don't look related to this change I think. (I'm not even sure the transitive include failures about thread missing cstdlib are related to this, but I needed those tests to pass and it looks reasonable that thread includes cstdlib.)

[Mordante]

In D148405#4284091, @iana wrote:

In D148405#4277800, @Mordante wrote:

In D148405#4274364, @iana wrote:

In D148405#4274218, @Mordante wrote:

In D148405#4271485, @iana wrote:

In D148405#4271084, @Mordante wrote:

Interesting that it fails with modules. In general we keep these transitive includes to avoid possibly breaking user code that depends on these transitive includes. (Note it's a bug in the user's code to depend on these includes, but this is not easily spotted.) However when it block us we are free to remove them. So no objection to the removal.

I expect this to fail in the CI since the transitive includes are not updated. Let me know if you need help with this.

This change needs to be mentioned in our release notes (docs/ReleaseNotes.rst) since it may affect users.

I'm not sure what I'm doing wrong, but check-cxx doesn't complain about the removed include. If I take algorithm -> chrono out of the csv files it also doesn't complain.

Check-cxx needs to be executed for every language version. If you intend to do more of these cleanups I can explain what to do. If it's only one it's easier for me to commandeer the revision and apply the fixes.

Ah. This was the only pure include cycle I found, but it's possible there will be some module cycles that still need to be broken up. It'd be good for me to know how to run it properly anyway, but also feel free to commandeer if you prefer.

Actually the method is described in libcxx/test/libcxx/transitive_includes.sh.cpp:45 re-generating the files is done by building the target libcxx-generate-files.

Thanks, I think I got the tests all squared away. I'm not sure what's up with the sanitizer and aix failures, but they don't look related to this change I think. (I'm not even sure the transitive include failures about thread missing cstdlib are related to this, but I needed those tests to pass and it looks reasonable that thread includes cstdlib.)

The AIX and sanitizers are unrelated. The cstdlib is related. I know it's not always clear. Before cstdlib was indirectly provided by chrono and not listed. Removing that include causes it to be attributed by a different header providing it to thread. I know this can be a bit confusion to people unfamiliar with it.

Thanks for the fixes, LGTM!

libcxx/docs/ReleaseNotes.rst

 
 - Several incidental transitive includes have been removed from libc++. Those
   includes are removed based on the language version used. Incidental transitive
   inclusions of the following headers have been removed:
 
   - C++2b: ``atomic``, ``bit``, ``cstdint``, ``cstdlib``, ``cstring``, ``initializer_list``, ``limits``, ``new``,
            ``stdexcept``, ``system_error``, ``type_traits``, ``typeinfo``
 
+- ``<algorithm>`` no longer includes ``<chrono>`` in any C++ version (it was prevously included in C++17 and earlier).
+
 - The headers ``<experimental/algorithm>`` and ``<experimental/functional>`` have been removed, since all the contents
   have been implemented in namespace ``std`` for at least two releases.
 
 - The formatter specialization ``template<size_t N> struct formatter<const charT[N], charT>``
   has been removed. Since libc++'s format library was marked experimental there
   is no backwards compatibility option. This specialization has been removed
   from the Standard since it was never used, the proper specialization to use
   instead is ``template<size_t N> struct formatter<charT[N], charT>``.

libcxx/include/algorithm

 
 // [algorithm.syn]
 #include <initializer_list>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
-#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 17
-#  include <chrono>
-#endif
-
 #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
 #  include <atomic>
 #  include <bit>
 #  include <concepts>
 #  include <cstdlib>
 #  include <cstring>
 #  include <iterator>
 #  include <memory>
 #  include <stdexcept>
 #  include <type_traits>
 #  include <utility>
 #endif
 
 #endif // _LIBCPP_ALGORITHM

libcxx/test/libcxx/transitive_includes/cxx03.csv

 algorithm atomic
 algorithm bit
-algorithm chrono
 algorithm climits
 algorithm concepts
 algorithm cstddef
 algorithm cstdint
 algorithm cstdlib
 algorithm cstring
 algorithm initializer_list
 algorithm iosfwd

 system_error type_traits
 system_error version
 thread array
 thread cerrno
 thread chrono
 thread compare
 thread cstddef
 thread cstdint
+thread cstdlib
 thread cstring
 thread ctime
 thread functional
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio

libcxx/test/libcxx/transitive_includes/cxx11.csv

 algorithm atomic
 algorithm bit
-algorithm chrono
 algorithm climits
 algorithm concepts
 algorithm cstddef
 algorithm cstdint
 algorithm cstdlib
 algorithm cstring
 algorithm initializer_list
 algorithm iosfwd

 system_error type_traits
 system_error version
 thread array
 thread cerrno
 thread chrono
 thread compare
 thread cstddef
 thread cstdint
+thread cstdlib
 thread cstring
 thread ctime
 thread functional
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio

libcxx/test/libcxx/transitive_includes/cxx14.csv

 algorithm atomic
 algorithm bit
-algorithm chrono
 algorithm climits
 algorithm concepts
 algorithm cstddef
 algorithm cstdint
 algorithm cstdlib
 algorithm cstring
 algorithm initializer_list
 algorithm iosfwd

 system_error type_traits
 system_error version
 thread array
 thread cerrno
 thread chrono
 thread compare
 thread cstddef
 thread cstdint
+thread cstdlib
 thread cstring
 thread ctime
 thread functional
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio

libcxx/test/libcxx/transitive_includes/cxx17.csv

 algorithm atomic
 algorithm bit
-algorithm chrono
 algorithm climits
 algorithm concepts
 algorithm cstddef
 algorithm cstdint
 algorithm cstdlib
 algorithm cstring
 algorithm initializer_list
 algorithm iosfwd

 system_error type_traits
 system_error version
 thread array
 thread cerrno
 thread chrono
 thread compare
 thread cstddef
 thread cstdint
+thread cstdlib
 thread cstring
 thread ctime
 thread functional
 thread iosfwd
 thread limits
 thread locale
 thread new
 thread ratio