This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen
Bug 1752 - A change to the C++ Standard will break some tests
Summary: A change to the C++ Standard will break some tests
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Tests (show other bugs)
Version: unspecified
Hardware: All All
: Normal Failed Unit Test
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.4
  Show dependency treegraph
 
Reported: 2019-09-26 16:12 UTC by Jonathan Caves
Modified: 2019-12-04 18:48 UTC (History)
3 users (show)



Attachments
Reduced test case (1.14 KB, text/plain)
2019-09-26 16:12 UTC, Jonathan Caves
no flags Details

Description Jonathan Caves 2019-09-26 16:12:47 UTC
Created attachment 953 [details]
Reduced test case

Hi: the C++20 the C++ committee has adopted P0929R2 (checking for abstract class types). This paper basically changes when a compiler checks for the use of an abstract class type. Previously it was an error to declare a function that had a parameter with a type that was an instance of an abstract class type - now it is an error to define or call such a function (i.e. it is not an error to just declare such a function). Note: this paper was adopted as a DR (defect report) and hence it applies to all versions of C++ not just C++20.

In the file ..\Eigen\src\test\meta.cpp there is code roughly like below (if you preprocess it):

namespace internal
{
	template<typename T>
	struct remove_reference {
		typedef T type;
	};
	
	template<typename T>
	struct remove_reference<T&> {
		typedef T type;
	};

	template<typename From, typename To>
	struct is_convertible_impl {
	private:
		struct any_conversion
		{
			template<typename T>
			any_conversion(const volatile T&);
			
			template<typename T>
			any_conversion(T&);
		};

		struct yes { int a[1]; };
		struct no  { int a[2]; };

		template<typename T>
		static yes test(T, int);

		template<typename T>
		static no  test(any_conversion, ...);

	public:
		static typename internal::remove_reference<From>::type* ms_from;

		enum {
			value = sizeof(test<To>(*ms_from, 0)) == sizeof(yes)
		};
	};

	template<typename From, typename To>
	struct is_convertible {
		enum {
			value = is_convertible_impl<From, To>::value
		};
	};
}

struct MyInterface {
	virtual void func() = 0;
	virtual ~MyInterface() {}
};

struct MyImpl : public MyInterface {
	void func() {}
};

static_assert(!internal::is_convertible<MyImpl, MyInterface>::value, "EIGEN_INTERNAL_ERROR_PLEASE_FILE_A_BUG_REPORT");

The problem is that now the attempt to specialize the static member function 'test' with 'MyInterface' is no longer by itself an error and hence SFINAE does not kick in and the overload resolution process ends up pick 'test(MyInterface, int)' as the best match. This function is then called which does cause an error (specifically not a SFINAE error) and hence the compiler emits an error message.

Thanks
Jonathan Caves
Comment 1 Christoph Hertzberg 2019-09-27 15:42:11 UTC
Not sure if there is a workaround (which works for C++03, since for C++11 we could simply use std::is_convertible anyway).

The easiest solution would be to just remove the test unless we are at least in C++11 mode. AFAIK, inside Eigen we never test conversions to abstract classes, so this should not be a problem.
Comment 2 Gael Guennebaud 2019-10-10 09:10:56 UTC
yes, I would also simply disable the test.
Comment 3 Gael Guennebaud 2019-10-10 09:50:45 UTC
There is at least one difference between our is_convertible and the std one:

    std::is_convertible<Matrix3f,Matrix3f&>::value

is true ou case, false with std::is_convertible. Not sure yet if this matters or not.

Meanwhile, I disabled the tests to be future proof (and present proof regarding latest MSVC releases):

https://bitbucket.org/eigen/eigen/commits/3d932de8d859/
Comment 4 Gael Guennebaud 2019-10-10 15:42:48 UTC
https://bitbucket.org/eigen/eigen/commits/362a8d333dca/
Summary:     Bug 1752: make is_convertible equivalent to the std c++11 equivalent and fallback to std::is_convertible when c++11 is enabled.
Comment 5 Nobody 2019-12-04 18:48:47 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1752.

Note You need to log in before you can comment on or make changes to this bug.