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

Message ID 20220830030615.29089-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: base: signal: Disable connect() for functor if args mismatch
Related show

Commit Message

Laurent Pinchart Aug. 30, 2022, 3:06 a.m. UTC 
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,

Comments

Nicolas Dufresne via libcamera-devel Aug. 31, 2022, 2:24 a.m. UTC | #1 
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@ideasonboard.com>

\o/

Reviewed-by: Paul Elder <paul.elder@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
Kieran Bingham Aug. 31, 2022, 10:17 a.m. UTC | #2 
Quoting Laurent Pinchart via libcamera-devel (2022-08-30 04:06:15)
> 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>

I can't really say I have 'reviewed' this, however I like what it
promises, and the change itself looks small and straightforward.

So because of unknown template magic, (that I could go read up on, but
I'm not going to right now), I'll just add this. I think it should go
in.

Acked-by: Kieran Bingham <kieran.bingham@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
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 31, 2022, 10:57 a.m. UTC | #3 
On Wed, Aug 31, 2022 at 11:17:32AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-08-30 04:06:15)
> > 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>
> 
> I can't really say I have 'reviewed' this, however I like what it
> promises, and the change itself looks small and straightforward.
> 
> So because of unknown template magic, (that I could go read up on, but
> I'm not going to right now), I'll just add this. I think it should go
> in.

:-)

I went through the process of debugging this with Paul, the knowledge we
gained should be shared. Let me give it a try.

The original error message from gcc is

../../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 (SignalTest::*)(int)>*)this)->libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_) (...)’
  146 |                         return func_(args...);
      |                                ~~~~~^~~~~~~~~

Let's focus on the error line first. The issue occurs at

../../include/libcamera/base/bound_method.h:146:37

and the error message is

error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in [...]

Let's look at the code on line 146:

template<typename T, typename R, typename Func, typename... Args>
class BoundMethodFunctor : public BoundMethodArgs<R, Args...>
{
public:
[...]
	R activate(Args... args, bool deleteMethod = false) override
	{
		if (!this->object_)
->			return func_(args...);

		auto pack = std::make_shared<PackType>(args...);
		bool sync = BoundMethodBase::activatePack(pack, deleteMethod);
		return sync ? pack->returnValue() : R();
	}
[...]
};

The compiler complains that `func_(args...)` is not a valid way to call
a pointer-to-member function. That's right, a pointer-to-member needs to
be called on a particular object instance, such as in
BoundMethodMember::activate():

	return (obj->*func_)(args...);

But why are we reaching BoundMethodFunctor::activate() in the first
place if func_ is a pointer-to-member, as BoundMethodMember should be
used in that case ? The answer lies in the rest of the error message:

[...] in ‘((libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>*)this)->libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>*)this)->libcamera::BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_) (...)’

Let's strip the libcamera:: namespace qualifier to make it easier to
read:

[...] in ‘((BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>*)this)->BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>::func_ (...)’ [...]

This means

[...] in ‘((Type *)this)->Type::func_ (...)’ [...]

with Type being

BoundMethodFunctor<SignalTest, void, void (SignalTest::*)(int)>

That's already more readable, and indeed, the code tries to call
func_(), which is a member variable of the BoundMethodFunctor class.

So why does the compiler pick BoundMethodFunctor if func_ is a
pointer-to-member ? Let's look at the error message from clang, which
brings more information:

 ../../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);

We can see that BoundMethodFunctor is used in Signal::connect(), on line
80 of signal.h (that information wasn't provided by gcc). The code reads

	template<typename T, typename Func,
		 std::enable_if_t<!std::is_base_of<Object, T>::value> * = nullptr>
	void connect(T *obj, Func func)
	{
		SignalBase::connect(new BoundMethodFunctor<T, void, Func, Args...>(obj, nullptr, func));
	}

We also have another version of connect, for pointer-to-member:

	template<typename T, typename R, std::enable_if_t<!std::is_base_of<Object, T>::value> * = nullptr>
	void connect(T *obj, R (T::*func)(Args...))
	{
		SignalBase::connect(new BoundMethodMember<T, R, Args...>(obj, nullptr, func));
	}

So why does the compiler pick the first one ? When performing overload
resolution (multiple functions with the same name), the compiler will
try to match the arguments passed to the function with the arguments
expected by each overload. Here we see that both overloads have the same
number of arguments, with a different type for the second argument. As
the functions are function templates, the compiler will then try to
perform template argument resolution. The caller reads

		signalVoid_.connect(this, &SignalTest::slotInteger1);

with "this" being an instance of SignalTest, and slotInteger1 being a
member function declared as

	void slotInteger1(int value);

T is thus resolved to SignalTest, that's easy. For the second argument,
`R (T::*func)(Args...)`, R is resolved to void, T matches the previous
resolution (as slotInteger1 is a member of SignalTest). Note that Args
is not a template argument of the function, but of the Signal class
itself:

template<typename... Args>
class Signal : public SignalBase
{
[...]
};

Let's now go back to the error message from clang, and in particular to

 ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect<SignalTest, void (SignalTest::*)(int), nullptr>' requested here

This shows that Signal is instantiated as Signal<>, which is right, in
test/signal.cpp we have

	Signal<> signalVoid_;

Args... thus resolves to an empty list, but that doesn't match the
arguments to slotInteger1 ! The compiler thus fails to perform template
argument resolution for the Signal::connect() overload that is meant to
handle pointer-to-member arguments, and thus ignores it. What does it
then do ? It picks another candidate for the overload resolution:

	template<typename T, typename Func,
		 std::enable_if_t<!std::is_base_of<Object, T>::value> * = nullptr>
	void connect(T *obj, Func func)
	{
		SignalBase::connect(new BoundMethodFunctor<T, void, Func, Args...>(obj, nullptr, func));
	}

There again it tries to perform template argument resolution. T matches
SignalTest as before, and Func happily matches (SignalTest::*)(int), a
pointer-to-member, even though the code has been written to expect a
functor (an object that can be directly called, such as a lambda, a
std::function or an instance of any class that has an operator()). The
overload is resolved, and the compiler continues by instantiating a
BoundMethodFunctor as instructed by the function. That's where
compilation fails, as BoundMethodFunctor tries to call the functor, and
a pointer-to-member can't be called that way.

What does this teach us?

- The solution was in the error message :-) Even without clang, gcc
  indicated

../../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 = {}]’:

  which shows a mismatch between the arguments to Func (int) and the
  Args (empty list {}). Disection error messages is scary, but if you
  split them across multiple lines and drop full namespace qualifiers,
  they become more readable.

- clang still gives better error messages than gcc, not so much related
  to the error itself in this case, but there's more information about
  the callers for error related to template argument resolution.

- This could have been avoided by proper usage of std::enable_if<> to
  ensure that the template arguments match the expected types.

> Acked-by: Kieran Bingham <kieran.bingham@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

Patch
diff mbox series 

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>
---
 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