Adopt LWG-3541 indirectly_readable_traits should be SFINAE-friendly for all types
#1992
Conversation
This makes indirectly_readable_traits SFINAE friendly and adds some tests for that Addresses microsoft#1965
StephanTLavavej
changed the title
indirectly_readable_traits should be SFINAE-friendly for all types
| STATIC_ASSERT(same_as<SFINAE<int>::value_type, int>); | ||
| STATIC_ASSERT(same_as<SFINAE<int const>::value_type, int>); | ||
| STATIC_ASSERT(same_as<SFINAE<int volatile>::value_type, int>); | ||
| STATIC_ASSERT(same_as<SFINAE<int const volatile>::value_type, int>); |
There was a problem hiding this comment.
Why not simply STATIC_ASSERT(same_as<indirectly_readable_traits<iterish<int meow>>, int>)? And don't we already have coverage for these cases elsewhere?
There was a problem hiding this comment.
Wasnt the whole point of the issue to ensure that it is SFINAE friendly?
AFAIK STATIC_ASSERT(same_as<indirectly_readable_traits<iterish<int meow>>, int>) does not check that?
There was a problem hiding this comment.
We can't determine if something is SFINAE-friendly with test cases that don't cause substitution failure. The iterish<float meow> test cases validate the point of the change; without it, has_member_value_type<indirectly_readable_traits<iterish<float meow>>> would produce a compile error instead of evaluating to false. These test cases are simply verifying that things that worked before continue to work, which - as I suggested - I suspect we have coverage of elsewhere.
There was a problem hiding this comment.
Interestingly we already have exactly that test directly above.
This suggests, that the change is not really needed. I have removed the duplicated tests
There was a problem hiding this comment.
Interestingly we already have exactly that test directly above. This suggests, that the change is not really needed. I have removed the duplicated tests
Oh 🤦. This is the issue I've been talking with our compiler devs and EDG about for the last couple of weeks: MSVC and Clang non-conformingly are treating ambiguity while choosing a partial template specialization as being a SFINAE-able error in cases where they should not be doing so. The test cases were passing because of the bug, but will need the product code change to keep working once the compiler bugs are fixed.
There was a problem hiding this comment.
I'm leaving this unresolved - despite that there's nothing here to resolve - to point out why we're adding no new test coverage in this PR.
|
Thanks for making the STL friendlier to SFINAE! 😻 🎉 🚀 |
No description provided.