Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move numerous components from <xstddef> to other headers and reduce inclusion #3623

Merged
merged 16 commits into from
Apr 14, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Apr 4, 2023

This PR is trying to reduce inclusion dependency. Towards #3599.

The most signification changes are about <xstddef>. I believe most of the contents of <xstddef> should be moved to other headers (especially <type_traits> and <xutility>), which is probably "pure win" for throughput.

Unfortunately, __builtin_bit_cast is used in <compare> instead of a wrapper function (or the standard function std::bit_cast), because I didn't find the right place for the real function.

I previously tried to remove <cwchar> dependency for <limits> in #3482, but that PR was blocked by some compiler bugs.

Edit: this PR is now only focusing on <xstddef>. I've reported DevCom-10331466 due to the failed/skipped libc++test for <stdlib.h>.

Driven-by change: reordering the entry for LWG-3865 in expected_results.txt.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner April 4, 2023 18:10
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Apr 4, 2023
@StephanTLavavej

This comment was marked as resolved.

@frederick-vs-ja

This comment was marked as resolved.

@frederick-vs-ja frederick-vs-ja changed the title <xstddef> etc.: Reduce inclusion dependency Move numerous components from <xstddef> to other headers and reduce inclusion Apr 5, 2023
stl/inc/functional Outdated Show resolved Hide resolved
stl/inc/functional Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Apr 9, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Apr 10, 2023

I've analyzed this PR's changes to understand their potential source-breaking impact.

  • _Unfancy() is moving from <xstddef> all the way down to <xmemory>.
    • This is an internal function, so there's no source-breaking impact.
    • <xmemory> is a good location - it's appropriate for fancy pointer manipulation, and it's included by all usage sites.
  • unary_function and binary_function are moving from <xstddef> all the way down to <functional>.
    • There's some potential source-breaking impact, as they're available in C++14 mode, and this is changing from "nearly ambiently available" to "must include the proper header". (In the entire STL at this time, only <future> includes <functional> as a whole.) I'm not too concerned, since they're removed by default in C++17 mode, so we've been discouraging use for a while. I think we can deal with the fallout of this change.
    • <functional> is the proper location, as no other header needs them.
  • The _MEMBER_CALL macro family and _Fake_copy_init() are moving from <xstddef> slightly down to <type_traits>.
    • Internal machinery, no source-breaking impact.
    • <type_traits> is a good location; this machinery isn't needed by the 2 headers that care about the difference (see below).
  • addressof(), less<_Ty>, and less<void> are moving from <xstddef> slightly down to <type_traits>.
    • This has very limited source-breaking impact - although less is extremely popular, this is still "nearly ambiently available". There are only 2 headers that included <xstddef> in main and don't include <type_traits> after this PR: <iosfwd> and <limits>. I think this is unlikely to be a problem.
    • <type_traits> is a good central location.
  • The primary templates and void specializations of plus, minus, multiplies, equal_to, not_equal_to, greater, greater_equal, and less_equal are moving from <xstddef> down to <xutility>.
    • While <xutility> is fairly popular, this affects a lot of headers. Specifically, the following headers included <xstddef> in main and don't include <xutility> after this PR: <atomic>, <bit>, <compare>, <concepts>, <coroutine>, <exception>, <iosfwd>, <latch>, <limits>, <new>, <numbers>, <ratio>, <semaphore>, <tuple>, <type_traits>, <typeindex>, <typeinfo>, and <utility>.
    • <xutility> is a good location, as none of those affected headers need these operator function objects.
    • Although this is all available in C++14 mode, I hope/expect that the source-breaking impact will be low. These function objects are less popular than less (presumably greater is the second-most popular) and <xutility> is still included widely enough that it should be fairly uncommon for a TU to be including only the affected subset. I'm willing to risk it and see what the fallout is.
  • <xstddef> no longer includes <cstdlib> and <initializer_list>.

In practice, the breaking change impact I've seen in the compiler's test suite has been:

  • Need to include <functional> for unary_function and binary_function.
  • Need to include <cstdlib> for things like exit and quick_exit.
  • Need to include <initializer_list> for initializer_list, sometimes used implicitly.

@strega-nil-ms strega-nil-ms self-assigned this Apr 11, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 90d87a5 into microsoft:main Apr 14, 2023
@StephanTLavavej
Copy link
Member

Thanks for improving the STL's throughput and helping users to be more disciplined about inclusion! This ended up being more source-breaking than I originally expected, but hopefully we can power through the resulting breaks and ship this in VS 2022 17.7. 🚀 🚢 😹

@kiwidev
Copy link
Member

kiwidev commented Aug 10, 2023

Given that this breaks code when updating to VS17.7, could a note be included inside the release notes for VS17.7 discussing the breaking change?
Having to hunt down and find this when a team is broken (and it will take time to move code changes through repos etc) is not a good experience for a component that is usually treated as stable and is automatically updated.

@frederick-vs-ja
Copy link
Contributor Author

Given that this breaks code when updating to VS17.7, could a note be included inside the release notes for VS17.7 discussing the breaking change?

This is already recorded in the Changelog, although it's not automatically merged into release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants