llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.4k stars 12.15k forks source link

sfinae discrepancy vs gcc #39709

Closed mibintc closed 5 years ago

mibintc commented 5 years ago
Bugzilla Link 40362
Resolution FIXED
Resolved on Jan 17, 2019 15:13
Version trunk
OS Windows NT
CC @DougGregor,@erichkeane,@zygoloid
Fixed by commit(s) r351495

Extended Description

This test case gets an error with clang but it's OK with gcc8. Is this a clang bug? clang++ -c -std=c++17 test.cpp test.cpp:14:75: warning: incompatible integer to pointer conversion assigning to 'int ' from 'long' [-Wint-conversion] ...U> typename enable_if<(sizeof(create_a() -= create_a(), 1) > 0), y... ^ ~~~~~ test.cpp:16:17: note: in instantiation of template class 'has_minus_assign<int , int >' requested here static_assert((!has_minus_assign<int>::value), "(!has_minus_assign<int... ^ test.cpp:16:1: error: static_assert failed due to requirement '!has_minus_assign<int , int >::value' "(!has_minus_assign<int>::value)" static_assert((!has_minus_assign<int>::value), "(!has_minus_assign<int... ^ ~~~~~~ 1 warning and 1 error generated. $ g++ -c -std=c++17 test.cpp $ cat test.cpp typedef char yes_type; struct no_type { char data[2]; }; template T create_a(); template struct type { }; template<bool, typename T = void> struct enable_if { typedef T type; }; template struct enable_if<false, T> { }; struct Y { Y& operator=(Y&); }; struct X { X& operator-=(X); }; struct Z { }; template<typename T, typename U> typename enable_if<(sizeof(create_a() -= create_a(), 1) > 0), yes_type>::type check_has_minus_assign(type, type); no_type check_has_minus_assign(...); template<typename T, typename U = T> struct has_minus_assign { static const bool value = (sizeof(check_has_minus_assign(type<T&>(), type())) == sizeof(yes_type)); }; X& operator-=(X&, Y); static_assert((!has_minus_assign<int>::value), "(!has_minus_assign<int>::value)");

--Melanie Blower I work for Intel on the C++ compiler

erichkeane commented 5 years ago

The right fix is to mark that ext_ diagnostic as SFINAEFailure. (That really should be the default for Extension and ExtWarn, but that change breaks a bunch of things in the testsuite and I've not had time to do the cleanup yet.)

Ah, wonderful! I'd not known about that. I'll do that after lunch.

Thanks Richard!

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 5 years ago

The right fix is to mark that ext_ diagnostic as SFINAEFailure. (That really should be the default for Extension and ExtWarn, but that change breaks a bunch of things in the testsuite and I've not had time to do the cleanup yet.)

erichkeane commented 5 years ago

A quick update... I could potentially limit my change to CPlusPlus (or CPlusPlus11), since none of my tests are C++ specific. This would leave the "extension" in place for C and OpenCL modes, but not break C++ SFINAE (as well as result in seemingly zero test failures).

Does anyone have a preference/suggestion other than this?

erichkeane commented 5 years ago

The issue here ends up being that Clang is more permissive, and the test case is using SFINAE to detect legality. IMO, we should make the below an error case. I tried just

https://godbolt.org/z/R343Hw

void foo(int a, int b) { a -= b; // note we allow this. int *c = a - b; // but not this? }

I attempted a "large hammer" fix for this (changing all of ext_typecheck_convert_int_pointer to an error), which fixes Melanie's example below (note, this is from the GCC test suite, so whoever fixes this should create a new test case to avoid license issues). However, it results in a large amount of test failures(~30), so I'm hoping someone can clarify whether this is the RIGHT fix, or too aggressive of one (perhaps suggesting an alternative).

If this is the right fix, I can spend the time to fix the tests that fail.