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

<__msvc_int128.hpp>: Remove unnecessary common_type partial specializations #3153

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

These partial specializations are unnecessary, because the default mechanisms (the conditional operator and implicit conversions) are already doing right things. It's surprising that the necessity of these specializations hasn't been discussed in #2518.

STL/stl/inc/__msvc_int128.hpp

Lines 1029 to 1043 in 1fe4d43

#ifdef __cpp_lib_concepts
template <integral _Ty>
struct common_type<_Ty, _Unsigned128> {
using type = _Unsigned128;
};
template <integral _Ty>
struct common_type<_Unsigned128, _Ty> {
using type = _Unsigned128;
};
#else // ^^^ defined(__cpp_lib_concepts) / !defined(__cpp_lib_concepts) vvv
template <class _Ty>
struct common_type<_Ty, _Unsigned128> : enable_if<is_integral_v<_Ty>, _Unsigned128> {};
template <class _Ty>
struct common_type<_Unsigned128, _Ty> : enable_if<is_integral_v<_Ty>, _Unsigned128> {};
#endif // ^^^ !defined(__cpp_lib_concepts) ^^^

STL/stl/inc/__msvc_int128.hpp

Lines 1430 to 1444 in 1fe4d43

#ifdef __cpp_lib_concepts
template <integral _Ty>
struct common_type<_Ty, _Signed128> {
using type = _Signed128;
};
template <integral _Ty>
struct common_type<_Signed128, _Ty> {
using type = _Signed128;
};
#else // ^^^ defined(__cpp_lib_concepts) / !defined(__cpp_lib_concepts) vvv
template <class _Ty>
struct common_type<_Ty, _Signed128> : enable_if<is_integral_v<_Ty>, _Signed128> {};
template <class _Ty>
struct common_type<_Signed128, _Ty> : enable_if<is_integral_v<_Ty>, _Signed128> {};
#endif // ^^^ !defined(__cpp_lib_concepts) ^^^

And the non-concept versions (added by me) may do evil - they may opt-out common types when there can be valid ones. Although this is probably not a conformance issue, as when concepts are not available, these integer-class types are just implementation details.

Ensuring that common_type specializations are not doing evil.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 12, 2022 03:19
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Good catch! I want to believe these were necessary for some prior formulation of the _Meow128 types, but that's certainly not true for their final form.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Oct 12, 2022
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Oct 12, 2022
@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Oct 12, 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.

Good catch!

@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 12, 2022
@StephanTLavavej StephanTLavavej self-assigned this Oct 21, 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 032aae1 into microsoft:main Oct 24, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 24, 2022
@StephanTLavavej
Copy link
Member

Thanks for tidying up the STL, and keeping only those partial specializations that bring us joy! 😻 😹 ✨

@frederick-vs-ja frederick-vs-ja deleted the simplified-common_type branch October 24, 2022 23:24
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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants