[libcxx] Fix PR26103 - Error calling is_convertible with incomplete type

[mdaniels]

Summary

Using std::is_convertible with a pointer to class which contains an incomplete type, and the is_convertible_to is not used (gcc / _LIBCPP_USE_IS_CONVERTIBLE_FALLBACK), is resulting in ADL. Making __test_convert qualified avoid this.

[mdaniels]

This changes was tested on a 64bit Linux host building llvm with gcc-5.3+libc++, and saw no change to regression runs with gcc-4.8.5, gcc-4.9.3, gcc-5.3, clang-3.4 and clang-3.7.1

[EricWF]

Thanks for the patch! The test case looks good and properly exercises the bug.

However there are a few things that need to be cleaned up. Sorry for the nit-picky review.

include/type_traits

1304

The problem here is that __test_convert is un-qualified so ADL is performed. This seems to instantiate Forward* as seen in your test. Fully qualifying this call has the same effect as your entire patch.

decltype(_VSTD::__is_convertible_imp::__test_convert<_To>(_VSTD::declval<_From>()))

Since I'm not sure that using a constructor is semantically equivalent using a function I would prefer this change instead.

test/std/utilities/meta/meta.rel/is_convertible.pass.cpp

59

This test type can be implemented in a clearer manner. The purpose of Forward is that it causes a compilation error when instantiated. However that's not immediately clear from the name or implementation.

I would go with something like:

template <typename T>
class CannotInstantiate {
  enum { X = T::ThisExpressionWillBlowUp };
};

205

This change is incorrect. We want to run this test in the following configurations:

• C++11 and beyond for all compilers.
• C++03 when using Clang's __is_convertible builtin.

The second case is handled by the bit of code you removed.
If your goal was to get this test working with libstdc++ I would change the line to:

#if __cplusplus >= 201103L || (defined(_LIBCPP_VERSION) && !defined(_LIBCPP_USE_IS_CONVERTIBLE_FALLBACK))

215

Can you describe what this test is doing a little more?

For example:

Ensure that Forward is not instantiated by is_convertible when it is not needed. For example Forward is instatiated as a part of ADL lookup for arguments of type Forward*.

[mdaniels]

include/type_traits

1304

Tests still pass with this, and looks way nicer, consider it done :)

test/std/utilities/meta/meta.rel/is_convertible.pass.cpp

205

I misread the code, I will revert this.

215

Sure thing, thanks for the suggestion.

[EricWF]

LGTM after addressing the IncompleteClass comment.

Thanks for the patch!

test/std/utilities/meta/meta.rel/is_convertible.pass.cpp

216

CannotInstantiate<int>* works too. Using IncompleteClass seems a little distracting because it has nothing to do with the class being incomplete.
The test will fail in the exact same way when a complete class type is given.

[mdaniels]

I dont have commit access, so if the change still looks good, and someone is feeling generous.. :)

[EricWF]

Committed as r258852. Thanks for the patch.

include/type_traits

 template <class _T1, class _T2> struct _LIBCPP_TYPE_VIS_ONLY is_convertible
     : public integral_constant<bool, __is_convertible_to(_T1, _T2) &&
                                      !is_abstract<_T2>::value> {};
 
 #else  // __has_feature(is_convertible_to)
 
 namespace __is_convertible_imp
 {
-template <class _Tp> void  __test_convert(_Tp);
+template <class _To>
+struct _Dst
+{
+    _Dst(_To);
+};
 
-template <class _From, class _To, class = void>
-struct __is_convertible_test : public false_type {};
+template <size_t> struct __one { typedef char type; };
+template <class _From, class _To> typename __one<sizeof(_Dst<_To>(declval<_From>()))>::type __test(int);
+template <class _From, class _To> __two __test(...);
 
 template <class _From, class _To>
-struct __is_convertible_test<_From, _To,
-    decltype(__test_convert<_To>(_VSTD::declval<_From>()))> : public true_type
-{};
+struct __is_convertible_test
+    : public integral_constant<bool, sizeof(__test<_From, _To>(0)) == 1> {};
 
-template <class _Tp> __two __test(...);
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
 template <class _Tp> _Tp&& __source();
 #else
 template <class _Tp> typename remove_reference<_Tp>::type& __source();
 #endif
 
 template <class _Tp, bool _IsArray =    is_array<_Tp>::value,
                      bool _IsFunction = is_function<_Tp>::value,

test/std/utilities/meta/meta.rel/is_convertible.pass.cpp

 
 typedef void Function();
 typedef char Array[1];
 
 class NonCopyable {
   NonCopyable(NonCopyable&);
 };
 
+class ForwardClass;
+template <typename T> class ForwardWrapper {
+  const T value;
+};
+typedef ForwardWrapper<ForwardClass> Forward;
+
 int main()
 {
     // void
     test_is_convertible<void,void> ();
     test_is_not_convertible<void,Function> ();
     test_is_not_convertible<void,Function&> ();
     test_is_not_convertible<void,Function*> ();
     test_is_not_convertible<void,Array> ();
int main()
     static_assert((std::is_convertible<NonCopyable&, const NonCopyable&>::value), "");
     static_assert((std::is_convertible<NonCopyable&, const volatile NonCopyable&>::value), "");
     static_assert((std::is_convertible<NonCopyable&, volatile NonCopyable&>::value), "");
     static_assert((std::is_convertible<const NonCopyable&, const NonCopyable&>::value), "");
     static_assert((std::is_convertible<const NonCopyable&, const volatile NonCopyable&>::value), "");
     static_assert((std::is_convertible<volatile NonCopyable&, const volatile NonCopyable&>::value), "");
     static_assert((std::is_convertible<const volatile NonCopyable&, const volatile NonCopyable&>::value), "");
     static_assert((!std::is_convertible<const NonCopyable&, NonCopyable&>::value), "");
-// This test requires Access control SFINAE which we only have in C++11 or when
-// we are using the compiler builtin for is_convertible.
-#if __cplusplus >= 201103L || !defined(_LIBCPP_USE_IS_CONVERTIBLE_FALLBACK)
+// This test requires Access control SFINAE which we only have in C++11
+#if __cplusplus >= 201103L
     test_is_not_convertible<NonCopyable&, NonCopyable>();
 #endif
 
+    // Forward
+    static_assert((std::is_convertible<Forward*, Forward*>::value), "");
 }