[libcxx-commits] [PATCH] D58987: Make underlying_type SFINAE-friendly

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists added inline comments.


================
Comment at: test/std/utilities/meta/meta.trans/meta.trans.other/underlying_type.pass.cpp:30
 
+template<class T>
+std::enable_if_t<
----------------
zoecarver wrote:
> mclow.lists wrote:
> > zoecarver wrote:
> > > mclow.lists wrote:
> > > > EricWF wrote:
> > > > > I'm not sure what this test is actually testing. How about:
> > > > > 
> > > > > ```
> > > > > template <class T, class = typename std::underlying_type<T>::type>
> > > > > std::true_type test_sfinae(int);
> > > > > template <class T>
> > > > > std::false_type test_sfinae(...);
> > > > > 
> > > > > int main() {
> > > > >   static_assert(decltype(test_sfinae<int>(0))::value == false, "not an enum");
> > > > >    static_assert(decltype(test_sfinae<E>(0))::value == true, "is an enum");
> > > > > 
> > > > > }
> > > > > ```
> > > > Now that we have the failing test, do we need this test at all?
> > > I like that idea @EricWF. 
> > > 
> > > Thinking more about how to structure these tests, I think the "fail" test is not super helpful (as is). What this should be testing for is whether passing `underlying_type` a non-enum type breaks the compiler. Given that it _shouldn't_ break the compiler, I think it is more helpful to make sure it passes than to try and make sure it fails in the "right" way. Let me know if you disagree though.
> > I don't know what you mean by "breaks the compiler" here.
> > 
> I mean before this patch, the below code could not compile. The compiler would error when it tried to interpret the `underlying_type` template:
> 
> ```
> template<class T>
> std::enable_if_t<
> std::is_same_v<std::underlying_type_t<T>, int>
> > test_sfinae(T t);
> 
> void test_sfinae(int) { return; }
> 
> int main()
> {
> 	test_sfinae(0); // Error while substituting deduced template arguments into function template 'test_sfinae' [with T = int]
> }
> ```
And you believe that the compiler was wrong to do so?
If so, have you filed a bug against clang?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58987/new/

https://reviews.llvm.org/D58987