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

<tuple>: Becomes a core header #2730

Merged
merged 13 commits into from
Jun 2, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented May 19, 2022

May be related to #1405.

I've found that <tuple> is nearly a core header in MSVC STL now, except that it requires allocator_arg and uses_allocator. IIUC neither allocator_arg nor uses_allocator needs any non-core mechanism, so I'm moving them from <xutility> to <utility>, and making <tuple> a core header.


In order to keep apply and make_from_tuple working with subrange, I decided to separate the needed parts of <xutility> into a new internal header <__msvc_iter_core.hpp>.

The intent is providing forward declaration of subrange and the get overloads, while avoiding separate the forward declaration of iterator_traits and its definition into different headers.

Internal-only changes seem required as in #2518.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner May 19, 2022 03:56
@frederick-vs-ja
Copy link
Contributor Author

Oh... I've realized that this PR may violate the direction I proposed in LWG-3690. Perhaps subrange should be forward-declared.

@frederick-vs-ja frederick-vs-ja marked this pull request as draft May 19, 2022 04:14
@frederick-vs-ja frederick-vs-ja marked this pull request as ready for review May 19, 2022 15:58
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label May 20, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good to me, personally, and I really like <tuple> becoming a core header; however, I don't personally understand the whole core distinction yet, and what that might cause, so I'd prefer a more experienced person on the team reviews this as well.

@CaseyCarter CaseyCarter added the decision needed We need to choose something before working on this label May 24, 2022
@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label May 25, 2022
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and we think that the benefits of making <tuple> a core header outweigh the (relatively small) costs involved. However, as the "core header" feature hasn't been clearly defined and documented, we consider making other headers into core headers a non-goal at this time - absent a motivating use case.

@StephanTLavavej StephanTLavavej self-assigned this May 25, 2022
@StephanTLavavej
Copy link
Member

I have verified that, with the exception of the new declarations, this PR is only moving code around and not changing it.

There's an observable header inclusion impact - previously, <tuple> dragged in <climits> and <cstring>, and now it doesn't (verified through actual compilation). This seems fine and fairly low impact. Because the new header includes <utility>, major impacts are avoided.

⚠️ Note to self:

As @frederick-vs-ja noted in the PR description, this requires (simple) MSVC-internal changes to add the new header.

I will additionally need to handle the code movement when rebasing my import-std branch. This is adding declarations of get(const subrange&) and get(subrange&&) to the new header.

@StephanTLavavej StephanTLavavej removed their assignment May 27, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 1, 2022
@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 87b1f5d into microsoft:main Jun 2, 2022
@StephanTLavavej
Copy link
Member

Thanks for this usability improvement! 🎉 😸 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants