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

Don't include <xmemory> in <optional> and <variant> #3624

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

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

Separated from #3623.

std::variant and std::optional forbid using an array type as component, so it's unnecessary to include <xmemory> to obtain _Destroy_in_place.

I think it's OK to write <xutility> for short. Several other headers, including conditionally included <compare>, are already handled in <xutility>. Edit: dropping this due to review comments.

Edit: I also need to reduce the numeric_limits depedency (needed for EDG?). I expect that the current approach is transient.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner April 5, 2023 06:24
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.

Seems reasonable, but I don't like removing explicit dependencies, personally (since the throughput gains are negligible).

stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the throughput Must compile faster label Apr 5, 2023
@@ -104,7 +104,7 @@ struct _Optional_destruct_base<_Ty, false> { // either contains a value of _Ty o

_CONSTEXPR20 ~_Optional_destruct_base() noexcept {
if (_Has_value) {
_Destroy_in_place(_Value);
_Value.~_Ty();
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: I observe that we're storing remove_cv_t<_Ty> _Value; and saying only ~_Ty(). This seems to be acceptable according to the Standardese, and all compilers accept, so I don't think we need to add remove_cv_t here.

@StephanTLavavej
Copy link
Member

Looks good to me, thanks! I think that the breaking change impact here is limited - <optional> and <variant> are C++17 (so legacy C++14 code is unaffected), and the difference between <xmemory> and just <xutility> isn't massive. <limits> as you found and <tuple> in C++20 mode seem notable but not highly problematic.

@StephanTLavavej StephanTLavavej self-assigned this Apr 7, 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 d494511 into microsoft:main Apr 7, 2023
@StephanTLavavej
Copy link
Member

Thanks for finding and implementing this throughput improvement! 🚀 🐇 🐆

@frederick-vs-ja frederick-vs-ja deleted the throughput-optional-variant branch April 8, 2023 05:18
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.

3 participants