[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

1350

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

1350

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 _Tp> void  __test_convert(_Tp);
 
 template <class _From, class _To, class = void>
 struct __is_convertible_test : public false_type {};
 
 template <class _From, class _To>
 struct __is_convertible_test<_From, _To,
-    decltype(__test_convert<_To>(_VSTD::declval<_From>()))> : public true_type
+    decltype(_VSTD::__is_convertible_imp::__test_convert<_To>(_VSTD::declval<_From>()))> : public true_type
 {};
 
-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&);
 };
 
+template <typename T>
+class CannotInstantiate {
+  enum { X = T::ThisExpressionWillBlowUp };
+};
+
 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&, 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)
     test_is_not_convertible<NonCopyable&, NonCopyable>();
 #endif
 
+    // Ensure that CannotInstantiate is not instantiated by is_convertible when it is not needed.
+    // For example CannotInstantiate is instatiated as a part of ADL lookup for arguments of type CannotInstantiate*.
+    static_assert((std::is_convertible<CannotInstantiate<int>*, CannotInstantiate<int>*>::value), "");
 }