From 48d7bb29bd69c260bc5f9c104f6270dad4564b0d Mon Sep 17 00:00:00 2001
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date: Tue, 30 Aug 2022 05:45:01 +0300
Subject: libcamera: base: signal: Disable connect() for functor if args
 mismatch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/base/signal.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

(limited to 'include')

diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h
index efb591bc..841e4b4c 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
-- 
cgit v1.2.1