[libcamera-devel] [PATCH] libcamera: base: signal: Disable connect() for functor if args mismatch

[paul.elder at ideasonboard.com]

Hi Laurent,

On Tue, Aug 30, 2022 at 06:06:15AM +0300, Laurent Pinchart wrote:
> If a pointer-to-member is passed to the Signal::connect() function with
> arguments that don't match the Signal type, the pointer-to-member
> version of connect() will not match during template argument resolution,
> but the functor version will. This results in a compilation error in the
> BoundMethodFunctor class, due to the pointer-to-member not being a
> functor and thus not being callable directly. The error messages are
> quite cryptic. With the following error applied,
> 
> diff --git a/test/signal.cpp b/test/signal.cpp
> index 5c6b304dac0b..6dd11ac45313 100644
> --- a/test/signal.cpp
> +++ b/test/signal.cpp
> @@ -107,6 +107,7 @@ protected:
>  		/* Test signal emission and reception. */
>  		called_ = false;
>  		signalVoid_.connect(this, &SignalTest::slotVoid);
> +		signalVoid_.connect(this, &SignalTest::slotInteger1);
>  		signalVoid_.emit();
> 
>  		if (!called_) {
> 
> gcc outputs
> 
> ../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor<T, R, Func, Args>::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:
> ../../include/libcamera/base/bound_method.h:143:4:   required from here
> ../../include/libcamera/base/bound_method.h:146:37: error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in ‘((libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>*)this)->libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTes
> t::*)(int)>*)this)->libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_) (...)’
>   146 |                         return func_(args...);
>       |                                ~~~~~^~~~~~~~~
> 
> and clang isn't much better:
> 
> ../../include/libcamera/base/bound_method.h:146:11: error: called object type 'void (SignalTest::*)(int)' is not a function or function pointer
>                         return func_(args...);
>                                ^~~~~
> ../../include/libcamera/base/bound_method.h:137:2: note: in instantiation of member function 'libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::activate' requested here
>         BoundMethodFunctor(T *obj, Object *object, Func func,
>         ^
> ../../include/libcamera/base/signal.h:80:27: note: in instantiation of member function 'libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::BoundMethodFunctor' requested here
>                 SignalBase::connect(new BoundMethodFunctor<T, void, Func, Args...>(obj, nullptr, func));
>                                         ^
> ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect<SignalTest, void (SignalTest::*)(int), nullptr>' requested here
>                 signalVoid_.connect(this, &SignalTest::slotInteger1);
>                             ^
> 
> Improve error reporting by disabling the functor version of connect()
> when the Func argument isn't invocable with the Signal arguments. gcc
> will then complain with
> 
> ../../test/signal.cpp:110:36: error: no matching function for call to ‘libcamera::Signal<>::connect(SignalTest*, void (SignalTest::*)(int))’
>   110 |                 signalVoid_.connect(this, &SignalTest::slotInteger1);
>       |                 ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> and clang with
> 
> ../../test/signal.cpp:110:15: error: no matching member function for call to 'connect'
>                 signalVoid_.connect(this, &SignalTest::slotInteger1);
>                 ~~~~~~~~~~~~^~~~~~~
> 
> which are more readable.
> 
> This change requires usage of std::is_invocable<>, which is only
> available starting in C++17. This is fine for usage of the Signal class
> within libcamera, as the project is compiled with C++17, but we try to
> keep the public API compatible C++14. Condition the additional checks
> based on the C++ version.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

\o/

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> ---
>  include/libcamera/base/signal.h | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h
> index efb591bc5073..841e4b4ca15c 100644
> --- a/include/libcamera/base/signal.h
> +++ b/include/libcamera/base/signal.h
> @@ -63,7 +63,11 @@ public:
>  
>  #ifndef __DOXYGEN__
>  	template<typename T, typename Func,
> -		 std::enable_if_t<std::is_base_of<Object, T>::value> * = nullptr>
> +		 std::enable_if_t<std::is_base_of<Object, T>::value
> +#if __cplusplus >= 201703L
> +				  && std::is_invocable_v<Func, Args...>
> +#endif
> +				  > * = nullptr>
>  	void connect(T *obj, Func func, ConnectionType type = ConnectionTypeAuto)
>  	{
>  		Object *object = static_cast<Object *>(obj);
> @@ -71,7 +75,11 @@ public:
>  	}
>  
>  	template<typename T, typename Func,
> -		 std::enable_if_t<!std::is_base_of<Object, T>::value> * = nullptr>
> +		 std::enable_if_t<!std::is_base_of<Object, T>::value
> +#if __cplusplus >= 201703L
> +				  && std::is_invocable_v<Func, Args...>
> +#endif
> +				  > * = nullptr>
>  #else
>  	template<typename T, typename Func>
>  #endif