[type_traits][parameter] Inconsistent boost::is_convertible between gcc and clang
I have been tracking down an issue with some of my code which compiles fine with gcc but not clang. It manifested as a failure in Boost.Parameter but I eventually pinned it down to an inconsistency in the behaviour of boost::is_convertible. The following program demonstrates the issue: ============== #include <boost/type_traits/is_convertible.hpp> #include <boost/static_assert.hpp> class A {}; int main() { BOOST_STATIC_ASSERT((boost::is_convertible<A, A&>::value)); } ============== This compiles fine under g++ (4.7.1), but the assertion fires under clang++ (r160940). This is not too surprising because is_convertible is implemented in completely different ways on the two compilers: - on clang it is in terms of the __is_convertible intrinsic, which behaves like std::is_convertible and fails because rvalue references don't convert to lvalue references. - on gcc it has a complex custom implementation which at some point performs add_reference<> on the first argument and thus determines that the conversion is OK. Reading the docs for boost::is_convertible I think the clang interpretation is closer to the intention, in which case the gcc implementation is wrong, and so is Boost.Parameter (which relies on this behaviour). On the other hand, I suspect there's probably other code out there that depends on this in a similar way to Boost.Parameter, so perhaps it would be safer to tweak the clang implementation instead, and deviate from std::is_convertible. Any thoughts? John Bytheway
On Wed, Aug 8, 2012 at 8:59 PM, John Bytheway <jbytheway+boost@gmail.com>wrote:
I have been tracking down an issue with some of my code which compiles fine with gcc but not clang. It manifested as a failure in Boost.Parameter but I eventually pinned it down to an inconsistency in the behaviour of boost::is_convertible. The following program demonstrates the issue:
============== #include <boost/type_traits/is_convertible.hpp> #include <boost/static_assert.hpp>
class A {};
int main() { BOOST_STATIC_ASSERT((boost::is_convertible<A, A&>::value)); } ==============
This compiles fine under g++ (4.7.1), but the assertion fires under clang++ (r160940).
I would consider clang correct and gcc incorrect (this is without consulting the documentation).
This is not too surprising because is_convertible is implemented in completely different ways on the two compilers:
- on clang it is in terms of the __is_convertible intrinsic, which behaves like std::is_convertible and fails because rvalue references don't convert to lvalue references.
- on gcc it has a complex custom implementation which at some point performs add_reference<> on the first argument and thus determines that the conversion is OK.
That appears to explain it; again, I would consider the add_reference incorrect and a limitation. If one wants to add a reference qualifier (when using what I consider a correct implementation of is_convertible), one may do so explicitly; but, in the case of gcc's implementation, one can't prevent the addition of reference qualifiers, so there's no way to test convertibility from rvalues. Reading the docs for boost::is_convertible I think the clang
interpretation is closer to the intention, in which case the gcc implementation is wrong, and so is Boost.Parameter (which relies on this behaviour).
On the other hand, I suspect there's probably other code out there that depends on this in a similar way to Boost.Parameter,
Well, *technically*, such other code would be depending on an *implementation*, let's say, "feature", that is, according to your reading, undocumented. so perhaps it would
be safer to tweak the clang implementation instead, and deviate from std::is_convertible.
Other code aside, I believe the correct thing to do would be to put the gcc implementation more in line with the clang, std, and Boost-documented behavior. Maybe make the change to the gcc implementation and see what fails within Boost first? Any thoughts?
I guess so :) - Jeff
On 10/08/12 23:57, Jeffrey Lee Hellrung, Jr. wrote:
On Wed, Aug 8, 2012 at 8:59 PM, John Bytheway <jbytheway+boost@gmail.com>wrote: <snip>
Reading the docs for boost::is_convertible I think the clang interpretation is closer to the intention, in which case the gcc implementation is wrong, and so is Boost.Parameter (which relies on this behaviour).
On the other hand, I suspect there's probably other code out there that depends on this in a similar way to Boost.Parameter,
Well, *technically*, such other code would be depending on an *implementation*, let's say, "feature", that is, according to your reading, undocumented.
Indeed. It would be nice, however, to have a clearer indication of what exactly the intended behaviour is. However this is resolved, I guess the docs should be cleaned up too.
so perhaps it would be safer to tweak the clang implementation instead, and deviate from std::is_convertible.
Other code aside, I believe the correct thing to do would be to put the gcc implementation more in line with the clang, std, and Boost-documented behavior. Maybe make the change to the gcc implementation and see what fails within Boost first?
Sounds reasonable. I'll see if I can at least run the type_traits tests and see what happens. John
(CC'ing John Maddock) "Non-intrinsic version of `boost::is_convertible<int, int&>` returns true_type" is a BUG. In Boost 1.46 and prior versions, the doc of is_convertible says If an imaginary lvalue of type From is convertible to type To then inherits from true_type, So the behavior is correct in these versions. But, in later versions, the doc says If an imaginary rvalue of type From is convertible to type To then inherits from true_type, Also, here is the "History" section of the doc Boost 1.47.0 * Breaking change: changed is_convertible to C++0x behaviour when possible. (TR1's is_convertible is equivalent to the old Boost one. C++11's is_convertible is different from them; see N2255.) So, in Boost 1.47 and later versions, `boost::is_convertible<int, int&>` should return false_type. Below, I'll show the bug fix using the non-intrinsic version for gcc. The bug is caused by the following code (Line 132): template <typename From, typename To> struct is_convertible_basic_impl { static typename add_rvalue_reference<From>::type _m_from; static bool const value = sizeof( boost::detail::checker<To>::_m_check(_m_from, 0) ) == sizeof(::boost::type_traits::yes_type); }; Here, `_m_from` is not treated as an rvalue. The correct code is template <typename From, typename To> struct is_convertible_basic_impl { static bool const value = sizeof( boost::detail::checker<To>::_m_check(boost::declval<From>(), 0) ) == sizeof(::boost::type_traits::yes_type); }; Also, `any_conversion` needs a constructor for an rvalue (Line 120): template <typename T> any_conversion(const T&); Finally, in `is_convertible_impl`, we shouldn't use `ref_type` in Line 299. Just using `From` is correct: boost::detail::is_convertible_basic_impl<From,To>::value With these changes, `boost::is_convertible<int, int&>` on gcc returns false_type. One note: The above code cannot deal with `boost::is_convertible<Func, Func&>`, where `Func` is a function type in a C++03 mode. This results in a compiler error. (In a C++11 mode, it returns true_type.) `boost::is_convertible<void, void>` results in a compiler error in both C++03 and C++11 modes. If these needs to be supported, we have to add special treatment codes. Regards, Michel
Michel Morin wrote:
One note: The above code cannot deal with `boost::is_convertible<Func, Func&>`, where `Func` is a function type in a C++03 mode. This results in a compiler error. (In a C++11 mode, it returns true_type.) `boost::is_convertible<void, void>` results in a compiler error in both C++03 and C++11 modes.
Oops, `boost::is_convertible<void, void>` (with the proposed fix) works fine in both C++03 and C++11 modes. Regards, Michel
"Non-intrinsic version of `boost::is_convertible<int, int&>` returns true_type" is a BUG.
Yes it's a bug, but whether it's completely fixable is another matter (see comments below).
So, in Boost 1.47 and later versions, `boost::is_convertible<int, int&>` should return false_type.
Right.
Below, I'll show the bug fix using the non-intrinsic version for gcc. The bug is caused by the following code (Line 132):
template <typename From, typename To> struct is_convertible_basic_impl { static typename add_rvalue_reference<From>::type _m_from; static bool const value = sizeof( boost::detail::checker<To>::_m_check(_m_from, 0) ) == sizeof(::boost::type_traits::yes_type); };
Here, `_m_from` is not treated as an rvalue. The correct code is
template <typename From, typename To> struct is_convertible_basic_impl { static bool const value = sizeof( boost::detail::checker<To>::_m_check(boost::declval<From>(), 0) ) == sizeof(::boost::type_traits::yes_type); };
Yep. Fixed in Trunk.
Also, `any_conversion` needs a constructor for an rvalue (Line 120):
template <typename T> any_conversion(const T&);
Yep, fixed in Trunk.
Finally, in `is_convertible_impl`, we shouldn't use `ref_type` in Line 299. Just using `From` is correct:
boost::detail::is_convertible_basic_impl<From,To>::value
With these changes, `boost::is_convertible<int, int&>` on gcc returns false_type.
It does, but it also terminally breaks C++03 compilers (not just gcc).
One note: The above code cannot deal with `boost::is_convertible<Func, Func&>`, where `Func` is a function type in a C++03 mode.
Not only that, it also breaks for abstract types, arrays, incomplete types, and anything else I've forgotten. I've fixed the issue for GCC in C++11 mode by forwarding to std::is_convertible, but it remains for now in C++03 mode. Note that other compilers are similarly effected *even when using a compiler intrinsic*, so for example given: 1) is_convertible<int&&, int&> 2) is_convertible<int, int&> MSVC fails (1) even though it's using a compiler intrinsic (and no we can't stop using it, that breaks other stuff). Intel on Win32 fails both 1 & 2, again using it's __is_convertible intrinsic. It seems to me that these related issues are basically unfixable without badly breaking something else, Regards, John.
John Maddock wrote:
Finally, in `is_convertible_impl`, we shouldn't use `ref_type` in Line 299. Just using `From` is correct:
boost::detail::is_convertible_basic_impl<From,To>::value
With these changes, `boost::is_convertible<int, int&>` on gcc returns false_type.
It does, but it also terminally breaks C++03 compilers (not just gcc).
One note: The above code cannot deal with `boost::is_convertible<Func, Func&>`, where `Func` is a function type in a C++03 mode.
Not only that, it also breaks for abstract types, arrays, incomplete types, and anything else I've forgotten.
I've fixed the issue for GCC in C++11 mode by forwarding to std::is_convertible, but it remains for now in C++03 mode.
I don't think tweaking `intrinsics.hpp` is a good idea since gcc does not implement the intrinsic yet. The following code works well (incl. your mentioned cases) both in C++03 and C++11 on gcc. template <typename From, typename To> struct is_convertible_basic_impl { #ifdef BOOST_NO_RVALUE_REFERENCES static typename ::boost::add_reference<From>::type _m_from; static bool const rval_to_nonconst_lval_ref_conv = ::boost::type_traits::ice_and< !(::boost::is_function<From>::value) // Note: an rvalue ref to function type is an lvalue , !(::boost::is_reference<From>::value) , ::boost::is_reference<To>::value , !(::boost::is_const<typename ::boost::remove_reference<To>::type>::value) >::value; static bool const value = ::boost::type_traits::ice_and< sizeof( boost::detail::checker<To>::_m_check(_m_from, 0) ) == sizeof(::boost::type_traits::yes_type) , !rval_to_nonconst_lval_ref_conv >::value; #else static bool const value = sizeof( boost::detail::checker<To>::_m_check(boost::declval<From>(), 0) ) == sizeof(::boost::type_traits::yes_type); #endif }; template <typename From, typename To> struct is_convertible_impl { BOOST_STATIC_CONSTANT(bool, value = (::boost::type_traits::ice_and< ::boost::type_traits::ice_or< ::boost::detail::is_convertible_basic_impl<From,To>::value, ::boost::is_void<To>::value >::value, ::boost::type_traits::ice_not< ::boost::type_traits::ice_or< ::boost::is_array<To>::value, ::boost::is_function<To>::value >::value >::value >::value) ); }; * The C++03 code emulates C++11's behavior. For example, `is_convertible<Abst, Abst const&>` returns true_type for an abstract type `Abst`. * The code fixes the bug in the current code for function types. For example, `is_convertible<Func, Func>` should return false_type for a function type `Func`. But the current code returns true_type.
Note that other compilers are similarly effected *even when using a compiler intrinsic*, so for example given:
1) is_convertible<int&&, int&> 2) is_convertible<int, int&>
MSVC fails (1) even though it's using a compiler intrinsic (and no we can't stop using it, that breaks other stuff). Intel on Win32 fails both 1 & 2, again using it's __is_convertible intrinsic.
I just tried MSVC's `__is_convertible_to` intrinsic. It's too buggy... Did someone make a bug report to MS? Regards, Michel
Michel Morin wrote:
The following code works well (incl. your mentioned cases) both in C++03 and C++11 on gcc. <snip> static bool const rval_to_nonconst_lval_ref_conv = ::boost::type_traits::ice_and< !(::boost::is_function<From>::value) // Note: an rvalue ref to function type is an lvalue , !(::boost::is_reference<From>::value) , ::boost::is_reference<To>::value , !(::boost::is_const<typename ::boost::remove_reference<To>::type>::value) >::value;
Sorry, the above code is wrong. It does not detect binding const volatile reference to an rvalue. Here is a right code: static bool const rval_to_nonconst_lval_ref_conv = ::boost::type_traits::ice_and< !(::boost::is_function<From>::value) // Note: an rvalue ref to function type is an lvalue , !(::boost::is_reference<From>::value) , ::boost::is_reference<To>::value , ::boost::type_traits::ice_or< !::boost::is_const<typename ::boost::remove_reference<To>::type>::value , ::boost::type_traits::ice_and< ::boost::is_const<typename ::boost::remove_reference<To>::type>::value , ::boost::is_volatile<typename ::boost::remove_reference<To>::type>::value >::value >::value >::value; Regards, Michel
The following code works well (incl. your mentioned cases) both in C++03 and C++11 on gcc. <snip> static bool const rval_to_nonconst_lval_ref_conv = ::boost::type_traits::ice_and< !(::boost::is_function<From>::value) // Note: an rvalue ref to function type is an lvalue , !(::boost::is_reference<From>::value) , ::boost::is_reference<To>::value , !(::boost::is_const<typename ::boost::remove_reference<To>::type>::value) >::value;
Sorry, the above code is wrong. It does not detect binding const volatile reference to an rvalue. Here is a right code:
static bool const rval_to_nonconst_lval_ref_conv = ::boost::type_traits::ice_and< !(::boost::is_function<From>::value) // Note: an rvalue ref to function type is an lvalue , !(::boost::is_reference<From>::value) , ::boost::is_reference<To>::value , ::boost::type_traits::ice_or< !::boost::is_const<typename ::boost::remove_reference<To>::type>::value , ::boost::type_traits::ice_and< ::boost::is_const<typename ::boost::remove_reference<To>::type>::value , ::boost::is_volatile<typename ::boost::remove_reference<To>::type>::value >::value >::value
::value;
Michel, Apologies for the delay, as I have no time right now, can you please let me have this as a patch, and verify our test suite passes? Any changes should also probably be dependent upon BOOST_NO_SFINAE_EXPR as well as I believe the technique depends upon that. Sorry for the abundance of caution, but something somewhere usually breaks when we mess with a header like that, and there's a limit to how many folks I'm willing to annoy to fix a corner case! ;-) Cheers, John.
Hi John,
Apologies for the delay, as I have no time right now, can you please let me have this as a patch, and verify our test suite passes?
There three fixes for `is_convertible<From, To>` in the new code. A. `To` is a function type. B. `From` is an rvalue in C++11. C. `From` is an rvalue in C++03. Patches (of the code and test) for A are attached in https://svn.boost.org/trac/boost/ticket/7246 Since it is trivial to fix, may I commit it? For B and C, I'll make patches one by one and then post a message (or file a ticket and attach a patch there).
Any changes should also probably be dependent upon BOOST_NO_SFINAE_EXPR as well as I believe the technique depends upon that.
The current implementation does not use BOOST_NO_SFINAE_EXPR. Do I need to use it for the new code? The new code does not use more compiler functionality than the current code. It uses the same functionality as the current one. Regards, Michel
Apologies for the delay, as I have no time right now, can you please let me have this as a patch, and verify our test suite passes?
There three fixes for `is_convertible<From, To>` in the new code. A. `To` is a function type. B. `From` is an rvalue in C++11. C. `From` is an rvalue in C++03.
Patches (of the code and test) for A are attached in https://svn.boost.org/trac/boost/ticket/7246 Since it is trivial to fix, may I commit it?
Looks OK, yes please do go ahead.
For B and C, I'll make patches one by one and then post a message (or file a ticket and attach a patch there).
Any changes should also probably be dependent upon BOOST_NO_SFINAE_EXPR as well as I believe the technique depends upon that.
The current implementation does not use BOOST_NO_SFINAE_EXPR. Do I need to use it for the new code? The new code does not use more compiler functionality than the current code. It uses the same functionality as the current one.
I'm not sure, the old code uses: sizeof(test_proc(variable)) The new one uses: sizeof(test_proc(function_call())) I'm not sure if that counts as a SFINAE expression or not, but I remember similar code causing problems with MSVC (and probably others) when it was tried in type traits in the past. So I'm being extra careful! Cheers, John.
John Maddock wrote:
There three fixes for `is_convertible<From, To>` in the new code. A. `To` is a function type. B. `From` is an rvalue in C++11. C. `From` is an rvalue in C++03.
Patches (of the code and test) for A are attached in https://svn.boost.org/trac/boost/ticket/7246 Since it is trivial to fix, may I commit it?
Looks OK, yes please do go ahead.
Done.
I'm not sure, the old code uses:
sizeof(test_proc(variable))
The new one uses:
sizeof(test_proc(function_call()))
I'm not sure if that counts as a SFINAE expression or not, but I remember similar code causing problems with MSVC (and probably others) when it was tried in type traits in the past. So I'm being extra careful!
I think if `sizeof(test_proc(variable))` works then `sizeof(test_proc(function_call()))` works too. But, OK, I don't know about VC++, so I use your code: `_m_check(static_cast<rvalue_type>(_m_from))`. I created a trac ticket at https://svn.boost.org/trac/boost/ticket/7251 (is_convertible works incorrectly for rvalue source types) and attached a patch for C++11 there https://svn.boost.org/trac/boost/attachment/ticket/7251/fix_is_convertible_f... I'll make a patch for C++03 after the patch for C++11 is accepted. Regards, Michel
On Wed, Aug 8, 2012 at 8:59 PM, John Bytheway <jbytheway+boost@gmail.com> wrote:
I have been tracking down an issue with some of my code which compiles fine with gcc but not clang. It manifested as a failure in Boost.Parameter but I eventually pinned it down to an inconsistency in the behaviour of boost::is_convertible. The following program
For curiosity, when using Boost.Parameter what error did you see? (Even if I understand the error was ultimately not there.)
demonstrates the issue:
============== #include <boost/type_traits/is_convertible.hpp> #include <boost/static_assert.hpp>
class A {};
int main() { BOOST_STATIC_ASSERT((boost::is_convertible<A, A&>::value)); } ==============
This compiles fine under g++ (4.7.1), but the assertion fires under clang++ (r160940).
This is not too surprising because is_convertible is implemented in completely different ways on the two compilers:
- on clang it is in terms of the __is_convertible intrinsic, which behaves like std::is_convertible and fails because rvalue references don't convert to lvalue references.
- on gcc it has a complex custom implementation which at some point performs add_reference<> on the first argument and thus determines that the conversion is OK.
Reading the docs for boost::is_convertible I think the clang interpretation is closer to the intention, in which case the gcc implementation is wrong, and so is Boost.Parameter (which relies on this behaviour).
On the other hand, I suspect there's probably other code out there that depends on this in a similar way to Boost.Parameter, so perhaps it would be safer to tweak the clang implementation instead, and deviate from std::is_convertible.
Any thoughts?
Thanks, --Lorenzo
On 11/08/12 21:26, Lorenzo Caminiti wrote:
On Wed, Aug 8, 2012 at 8:59 PM, John Bytheway <jbytheway+boost@gmail.com> wrote:
I have been tracking down an issue with some of my code which compiles fine with gcc but not clang. It manifested as a failure in Boost.Parameter but I eventually pinned it down to an inconsistency in the behaviour of boost::is_convertible. The following program
For curiosity, when using Boost.Parameter what error did you see? (Even if I understand the error was ultimately not there.)
I've investigated further and am less sure that it was Boost.Parameter's fault. I attach my test case. In particular: BOOST_PARAMETER_CONSTRUCTOR( B, (constructor_impl), tag, (required (in_out(tag_1), (A&)) ) ) On closer reading of the Boost.Parameter docs I guess it is probably not allowed to pass a reference type as an type requirement, although again it's not entirely clear. I guess (not sure) that when I first wrote this code I used A in place of A&, which fails because A is non-moveable, and when Boost.Parameter checks is_convertible<A,A>, which is false, it thinks that the call is not a match. With the implementation of is_convertible at that time, switching to A& made everything work, because then it was checking is_convertible<A,A&>, which was true. Now my workaround no longer works, and I wonder: what is the proper way to capture a reference to a non-moveable type in a Boost.Parameter argument? Perhaps the A& technique I used is supposed to work but no longer does because of the changed meaning of boost::is_convertible after Boost 1.46 (as mentioned by Michel elsewhere)? John
participants (5)
-
Jeffrey Lee Hellrung, Jr. -
John Bytheway -
John Maddock -
Lorenzo Caminiti -
Michel Morin