[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

69–70 ↗

(On Diff #513936)

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/include/algorithm

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 #if defined(_LIBCPP_HAS_PARALLEL_ALGORITHMS) && _LIBCPP_STD_VER >= 17
 #  include <__pstl_algorithm>
 #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