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

Implement LWG-3545 std::pointer_traits should be SFINAE-friendly #3242

Merged
merged 3 commits into from Dec 6, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

Fixes #3237.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner November 21, 2022 16:36
Comment on lines +291 to +300
template <class, class = void, class = void>
struct _Ptr_traits_sfinae_layer {};

template <class _Ty, class _Uty>
struct _Ptr_traits_sfinae_layer<_Ty, _Uty, void_t<typename _Get_first_parameter<_Ty>::type>>
: _Ptr_traits_base<_Ty, typename _Get_first_parameter<_Ty>::type> {};

template <class _Ty>
struct _Ptr_traits_sfinae_layer<_Ty, void_t<typename _Ty::element_type>, void>
: _Ptr_traits_base<_Ty, typename _Ty::element_type> {};
Copy link
Contributor

Choose a reason for hiding this comment

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

So this seems to basically do as follows:

The third template specialization (right line 298) is the most specific of the three, so if it's valid, it'll be chosen, and our element_type will be _Ty::element_type.

If the third template specialization is not chosen, because _Ty::element_type is not a type name, then the second (right line 294) is still more specific than the first (right line 291), and if it is valid, we'll choose it.

Finally, otherwise, if _Ty is not a class template with only type arguments, we fall back to having no members.

This makes sense, and I think correctly implements the wording in the standard.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I believe it could have been written with both void_ts in the same position, but this way is fine too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't necessarily believe this is true, since if typename _Get_first_parameter<_Ty>::type and typename _Ty::element_type are both well formed (which they often are, like allocator<_Ty>), then both specializations are valid and equally specialized.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, what I'm envisioning is having the partial specializations be <_Ty, void_t<ONE>, _Uty> and <_Ty, void_t<TWO>, void>. In that case, when ONE and TWO are both well-formed, the partial specialization with void as the last argument is more specialized than the one with _Uty as the last argument.

@strega-nil-ms strega-nil-ms added this to Initial Review in Code Reviews via automation Nov 22, 2022
@strega-nil-ms strega-nil-ms added the LWG Library Working Group issue label Nov 22, 2022
@strega-nil-ms strega-nil-ms moved this from Initial Review to Final Review in Code Reviews Nov 22, 2022
@StephanTLavavej StephanTLavavej self-assigned this Nov 30, 2022
Comment on lines +291 to +300
template <class, class = void, class = void>
struct _Ptr_traits_sfinae_layer {};

template <class _Ty, class _Uty>
struct _Ptr_traits_sfinae_layer<_Ty, _Uty, void_t<typename _Get_first_parameter<_Ty>::type>>
: _Ptr_traits_base<_Ty, typename _Get_first_parameter<_Ty>::type> {};

template <class _Ty>
struct _Ptr_traits_sfinae_layer<_Ty, void_t<typename _Ty::element_type>, void>
: _Ptr_traits_base<_Ty, typename _Ty::element_type> {};
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I believe it could have been written with both void_ts in the same position, but this way is fine too.


STATIC_ASSERT(is_same_v<pointer_traits<Templated<char>>::element_type, char>);
STATIC_ASSERT(is_same_v<pointer_traits<Templated<char>>::pointer, Templated<char>>);
STATIC_ASSERT(is_same_v<pointer_traits<Templated<char>>::difference_type, char>);
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: Because this test case says using difference_type = I;, it's unable to distinguish whether pointer_traits is reading the template argument (as it did for element_type) or the difference_type typedef (as desired, which it is indeed doing). Using a unique type for the difference_type, like int, would make the test slightly stronger and clearer.

Below, CheckPriority's using element_type = T[42]; is a good example of detecting what pointer_traits is looking at.

I mention this only for future reference, as there is essentially zero risk in this specific case. This is related to my general recommendation for test data, which is "unique numbers for unique purposes".

@StephanTLavavej StephanTLavavej removed their assignment Dec 5, 2022
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Dec 5, 2022
@StephanTLavavej StephanTLavavej self-assigned this Dec 5, 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 0a1c6c4 into microsoft:main Dec 6, 2022
Code Reviews automation moved this from Ready To Merge to Done Dec 6, 2022
@StephanTLavavej
Copy link
Member

Thanks for helping SFINAE be everyone's friend! 😻 ✅ 🚀

@frederick-vs-ja frederick-vs-ja deleted the lwg-3545 branch December 7, 2022 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LWG Library Working Group issue
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

LWG-3545 std::pointer_traits should be SFINAE-friendly
3 participants