[{"id":24847,"web_url":"https://patchwork.libcamera.org/comment/24847/","msgid":"<20220831022418.GB15696@pyrite.rasen.tech>","date":"2022-08-31T02:24:18","subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent,\n\nOn Tue, Aug 30, 2022 at 06:06:15AM +0300, Laurent Pinchart wrote:\n> If a pointer-to-member is passed to the Signal::connect() function with\n> arguments that don't match the Signal type, the pointer-to-member\n> version of connect() will not match during template argument resolution,\n> but the functor version will. This results in a compilation error in the\n> BoundMethodFunctor class, due to the pointer-to-member not being a\n> functor and thus not being callable directly. The error messages are\n> quite cryptic. With the following error applied,\n> \n> diff --git a/test/signal.cpp b/test/signal.cpp\n> index 5c6b304dac0b..6dd11ac45313 100644\n> --- a/test/signal.cpp\n> +++ b/test/signal.cpp\n> @@ -107,6 +107,7 @@ protected:\n> \t\t/* Test signal emission and reception. */\n> \t\tcalled_ = false;\n> \t\tsignalVoid_.connect(this, &SignalTest::slotVoid);\n> +\t\tsignalVoid_.connect(this, &SignalTest::slotInteger1);\n> \t\tsignalVoid_.emit();\n> \n> \t\tif (!called_) {\n> \n> gcc outputs\n> \n> ../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:\n> ../../include/libcamera/base/bound_method.h:143:4: required from here\n> ../../include/libcamera/base/bound_method.h:146:37: error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in ‘((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor t::*)(int)>*)this)->libcamera::BoundMethodFunctor::func_) (...)’\n> 146 | return func_(args...);\n> | ~~~~~^~~~~~~~~\n> \n> and clang isn't much better:\n> \n> ../../include/libcamera/base/bound_method.h:146:11: error: called object type 'void (SignalTest::*)(int)' is not a function or function pointer\n> return func_(args...);\n> ^~~~~\n> ../../include/libcamera/base/bound_method.h:137:2: note: in instantiation of member function 'libcamera::BoundMethodFunctor::activate' requested here\n> BoundMethodFunctor(T *obj, Object *object, Func func,\n> ^\n> ../../include/libcamera/base/signal.h:80:27: note: in instantiation of member function 'libcamera::BoundMethodFunctor::BoundMethodFunctor' requested here\n> SignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n> ^\n> ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect' requested here\n> signalVoid_.connect(this, &SignalTest::slotInteger1);\n> ^\n> \n> Improve error reporting by disabling the functor version of connect()\n> when the Func argument isn't invocable with the Signal arguments. gcc\n> will then complain with\n> \n> ../../test/signal.cpp:110:36: error: no matching function for call to ‘libcamera::Signal<>::connect(SignalTest*, void (SignalTest::*)(int))’\n> 110 | signalVoid_.connect(this, &SignalTest::slotInteger1);\n> | ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> and clang with\n> \n> ../../test/signal.cpp:110:15: error: no matching member function for call to 'connect'\n> signalVoid_.connect(this, &SignalTest::slotInteger1);\n> ~~~~~~~~~~~~^~~~~~~\n> \n> which are more readable.\n> \n> This change requires usage of std::is_invocable<>, which is only\n> available starting in C++17. This is fine for usage of the Signal class\n> within libcamera, as the project is compiled with C++17, but we try to\n> keep the public API compatible C++14. Condition the additional checks\n> based on the C++ version.\n> \n> Signed-off-by: Laurent Pinchart \n\n\\o/\n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/base/signal.h | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h\n> index efb591bc5073..841e4b4ca15c 100644\n> --- a/include/libcamera/base/signal.h\n> +++ b/include/libcamera/base/signal.h\n> @@ -63,7 +63,11 @@ public:\n> \n> #ifndef __DOXYGEN__\n> \ttemplate -\t\t std::enable_if_t::value> * = nullptr>\n> +\t\t std::enable_if_t::value\n> +#if __cplusplus >= 201703L\n> +\t\t\t\t && std::is_invocable_v\n> +#endif\n> +\t\t\t\t > * = nullptr>\n> \tvoid connect(T *obj, Func func, ConnectionType type = ConnectionTypeAuto)\n> \t{\n> \t\tObject *object = static_cast(obj);\n> @@ -71,7 +75,11 @@ public:\n> \t}\n> \n> \ttemplate -\t\t std::enable_if_t::value> * = nullptr>\n> +\t\t std::enable_if_t::value\n> +#if __cplusplus >= 201703L\n> +\t\t\t\t && std::is_invocable_v\n> +#endif\n> +\t\t\t\t > * = nullptr>\n> #else\n> \ttemplate\n> #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D936AC0DA4\n\tfor ;\n\tWed, 31 Aug 2022 02:24:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A10761FBD;\n\tWed, 31 Aug 2022 04:24:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 489E261F9B\n\tfor ;\n\tWed, 31 Aug 2022 04:24:26 +0200 (CEST)","from pyrite.rasen.tech (unknown [50.228.9.220])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E4EEB481;\n\tWed, 31 Aug 2022 04:24:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661912668;\n\tbh=t4M9vzmgHOpji9Al6vUvFkqMo/lkXfiLBGNecmwnFcM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=w+oQk0sUHoV8i8gSe1PdVTnVSVRlsgpo3zAdqd3/GKBNKIuYu2l96jbjg8lqdqIKX\n\tbOw6jh4CCH8wkmv84YH3KJNYmVueGAsNGIcUP8akBCqJohKwb9UIFxxzt++m7vpByx\n\tsXQcXgxifrq9VRkW7ivtL/CjrVDin2KF0fU2CcX8buJSg3PEdqYlMueducWVapu9LJ\n\tAWposkcCGKtzno9oLAWLOUa1jxy7skdxgA7s/nhV7J9/WSn2s3TcAji5mSbbtKYwKG\n\tWePYP06VRXCQMh8lDmFDW3CPRyv59mg4GKFmVyhcja9uBCuXyFnxhZ4A3GlVRP9qs2\n\tVbxF61PUA+H+Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661912665;\n\tbh=t4M9vzmgHOpji9Al6vUvFkqMo/lkXfiLBGNecmwnFcM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mz6ZHJ8Q+HowtiMGdAvJgIilPIqRjTb2gk+8gfxiRN4P/FP+dBDKEC0nqref6Wieh\n\tqDmXZdRxMOl8yFxIsQimHrHfkCNcJDLfDDKRht0aTRxSY8g3CZb3nDeDE+V5cMstX2\n\tp6/G+awqEd4wdYRdwEL4howynCegR0ILodzxtwNc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Mz6ZHJ8Q\"; dkim-atps=neutral","Date":"Tue, 30 Aug 2022 21:24:18 -0500","To":"Laurent Pinchart ","Message-ID":"<20220831022418.GB15696@pyrite.rasen.tech>","References":"<20220830030615.29089-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20220830030615.29089-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24862,"web_url":"https://patchwork.libcamera.org/comment/24862/","msgid":"<166194105211.15972.2186307564879666027@Monstersaurus>","date":"2022-08-31T10:17:32","subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart via libcamera-devel (2022-08-30 04:06:15)\n> If a pointer-to-member is passed to the Signal::connect() function with\n> arguments that don't match the Signal type, the pointer-to-member\n> version of connect() will not match during template argument resolution,\n> but the functor version will. This results in a compilation error in the\n> BoundMethodFunctor class, due to the pointer-to-member not being a\n> functor and thus not being callable directly. The error messages are\n> quite cryptic. With the following error applied,\n> \n> diff --git a/test/signal.cpp b/test/signal.cpp\n> index 5c6b304dac0b..6dd11ac45313 100644\n> --- a/test/signal.cpp\n> +++ b/test/signal.cpp\n> @@ -107,6 +107,7 @@ protected:\n> /* Test signal emission and reception. */\n> called_ = false;\n> signalVoid_.connect(this, &SignalTest::slotVoid);\n> + signalVoid_.connect(this, &SignalTest::slotInteger1);\n> signalVoid_.emit();\n> \n> if (!called_) {\n> \n> gcc outputs\n> \n> ../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:\n> ../../include/libcamera/base/bound_method.h:143:4: required from here\n> ../../include/libcamera/base/bound_method.h:146:37: error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in ‘((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor t::*)(int)>*)this)->libcamera::BoundMethodFunctor::func_) (...)’\n> 146 | return func_(args...);\n> | ~~~~~^~~~~~~~~\n> \n> and clang isn't much better:\n> \n> ../../include/libcamera/base/bound_method.h:146:11: error: called object type 'void (SignalTest::*)(int)' is not a function or function pointer\n> return func_(args...);\n> ^~~~~\n> ../../include/libcamera/base/bound_method.h:137:2: note: in instantiation of member function 'libcamera::BoundMethodFunctor::activate' requested here\n> BoundMethodFunctor(T *obj, Object *object, Func func,\n> ^\n> ../../include/libcamera/base/signal.h:80:27: note: in instantiation of member function 'libcamera::BoundMethodFunctor::BoundMethodFunctor' requested here\n> SignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n> ^\n> ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect' requested here\n> signalVoid_.connect(this, &SignalTest::slotInteger1);\n> ^\n> \n> Improve error reporting by disabling the functor version of connect()\n> when the Func argument isn't invocable with the Signal arguments. gcc\n> will then complain with\n> \n> ../../test/signal.cpp:110:36: error: no matching function for call to ‘libcamera::Signal<>::connect(SignalTest*, void (SignalTest::*)(int))’\n> 110 | signalVoid_.connect(this, &SignalTest::slotInteger1);\n> | ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> and clang with\n> \n> ../../test/signal.cpp:110:15: error: no matching member function for call to 'connect'\n> signalVoid_.connect(this, &SignalTest::slotInteger1);\n> ~~~~~~~~~~~~^~~~~~~\n> \n> which are more readable.\n> \n> This change requires usage of std::is_invocable<>, which is only\n> available starting in C++17. This is fine for usage of the Signal class\n> within libcamera, as the project is compiled with C++17, but we try to\n> keep the public API compatible C++14. Condition the additional checks\n> based on the C++ version.\n> \n> Signed-off-by: Laurent Pinchart \n\nI can't really say I have 'reviewed' this, however I like what it\npromises, and the change itself looks small and straightforward.\n\nSo because of unknown template magic, (that I could go read up on, but\nI'm not going to right now), I'll just add this. I think it should go\nin.\n\nAcked-by: Kieran Bingham \n\n> ---\n> include/libcamera/base/signal.h | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h\n> index efb591bc5073..841e4b4ca15c 100644\n> --- a/include/libcamera/base/signal.h\n> +++ b/include/libcamera/base/signal.h\n> @@ -63,7 +63,11 @@ public:\n> \n> #ifndef __DOXYGEN__\n> template - std::enable_if_t::value> * = nullptr>\n> + std::enable_if_t::value\n> +#if __cplusplus >= 201703L\n> + && std::is_invocable_v\n> +#endif\n> + > * = nullptr>\n> void connect(T *obj, Func func, ConnectionType type = ConnectionTypeAuto)\n> {\n> Object *object = static_cast(obj);\n> @@ -71,7 +75,11 @@ public:\n> }\n> \n> template - std::enable_if_t::value> * = nullptr>\n> + std::enable_if_t::value\n> +#if __cplusplus >= 201703L\n> + && std::is_invocable_v\n> +#endif\n> + > * = nullptr>\n> #else\n> template\n> #endif\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AD4D6C3272\n\tfor ;\n\tWed, 31 Aug 2022 10:17:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E3D561FC0;\n\tWed, 31 Aug 2022 12:17:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A21361F9F\n\tfor ;\n\tWed, 31 Aug 2022 12:17:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E5EF5481;\n\tWed, 31 Aug 2022 12:17:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661941056;\n\tbh=4m/dRADn0JUfYj0AX/ZgwBE3iPb94E4FabNFt1ehstc=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=kl053YtMP1DxuR/fEZrdW7xo3bya8KCq15CqIh5jmpO1+BdDsTfbqRcwY5VyuFPqP\n\tvQwK9GP/BVxSytEOmkxf2N7LSEKDSH5cV2R5VIxU9slEqNnPWQRMZigNnz18xlIKfR\n\tCgStOJXvlaH27lDWUrLZdD3xys9hu65nwbr7lqnSd3cclYhsuEafp+LvvvvnIrm1SO\n\t8Uhq/jx+0TdDU7pZGowc6qY68jdtLzNEdEv6AAsylFXLbwkfjzwasfS1NC2tln3I6Y\n\tWdhuxwWrVSbJgXXasN349QQxUcxxrj4Iw2BfeYdjVE6d7IlmXGfPLosUIBu5SkuSyJ\n\t3x2D+AG0KRrFA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661941055;\n\tbh=4m/dRADn0JUfYj0AX/ZgwBE3iPb94E4FabNFt1ehstc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=LkU8SkhB/xtwqMZOsABnwsvcg6E/TLPOn0acCZac5xzga3A4fGWZ6CnT8GBEMqQ3N\n\tEaOXey7NjaRLcpm45wxIceWAsPZuOBjzAl2OTfrdGtrxpNqWiEIe8L97tKpX6x6ao3\n\t+7xJCS8mEKd2X0XllBDZawsmkykzSYTPNtIzW1rM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"LkU8SkhB\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220830030615.29089-1-laurent.pinchart@ideasonboard.com>","References":"<20220830030615.29089-1-laurent.pinchart@ideasonboard.com>","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 31 Aug 2022 11:17:32 +0100","Message-ID":"<166194105211.15972.2186307564879666027@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24863,"web_url":"https://patchwork.libcamera.org/comment/24863/","msgid":"","date":"2022-08-31T10:57:51","subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Aug 31, 2022 at 11:17:32AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart via libcamera-devel (2022-08-30 04:06:15)\n> > If a pointer-to-member is passed to the Signal::connect() function with\n> > arguments that don't match the Signal type, the pointer-to-member\n> > version of connect() will not match during template argument resolution,\n> > but the functor version will. This results in a compilation error in the\n> > BoundMethodFunctor class, due to the pointer-to-member not being a\n> > functor and thus not being callable directly. The error messages are\n> > quite cryptic. With the following error applied,\n> > \n> > diff --git a/test/signal.cpp b/test/signal.cpp\n> > index 5c6b304dac0b..6dd11ac45313 100644\n> > --- a/test/signal.cpp\n> > +++ b/test/signal.cpp\n> > @@ -107,6 +107,7 @@ protected:\n> > /* Test signal emission and reception. */\n> > called_ = false;\n> > signalVoid_.connect(this, &SignalTest::slotVoid);\n> > + signalVoid_.connect(this, &SignalTest::slotInteger1);\n> > signalVoid_.emit();\n> > \n> > if (!called_) {\n> > \n> > gcc outputs\n> > \n> > ../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:\n> > ../../include/libcamera/base/bound_method.h:143:4: required from here\n> > ../../include/libcamera/base/bound_method.h:146:37: error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in ‘((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor > t::*)(int)>*)this)->libcamera::BoundMethodFunctor::func_) (...)’\n> > 146 | return func_(args...);\n> > | ~~~~~^~~~~~~~~\n> > \n> > and clang isn't much better:\n> > \n> > ../../include/libcamera/base/bound_method.h:146:11: error: called object type 'void (SignalTest::*)(int)' is not a function or function pointer\n> > return func_(args...);\n> > ^~~~~\n> > ../../include/libcamera/base/bound_method.h:137:2: note: in instantiation of member function 'libcamera::BoundMethodFunctor::activate' requested here\n> > BoundMethodFunctor(T *obj, Object *object, Func func,\n> > ^\n> > ../../include/libcamera/base/signal.h:80:27: note: in instantiation of member function 'libcamera::BoundMethodFunctor::BoundMethodFunctor' requested here\n> > SignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n> > ^\n> > ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect' requested here\n> > signalVoid_.connect(this, &SignalTest::slotInteger1);\n> > ^\n> > \n> > Improve error reporting by disabling the functor version of connect()\n> > when the Func argument isn't invocable with the Signal arguments. gcc\n> > will then complain with\n> > \n> > ../../test/signal.cpp:110:36: error: no matching function for call to ‘libcamera::Signal<>::connect(SignalTest*, void (SignalTest::*)(int))’\n> > 110 | signalVoid_.connect(this, &SignalTest::slotInteger1);\n> > | ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > \n> > and clang with\n> > \n> > ../../test/signal.cpp:110:15: error: no matching member function for call to 'connect'\n> > signalVoid_.connect(this, &SignalTest::slotInteger1);\n> > ~~~~~~~~~~~~^~~~~~~\n> > \n> > which are more readable.\n> > \n> > This change requires usage of std::is_invocable<>, which is only\n> > available starting in C++17. This is fine for usage of the Signal class\n> > within libcamera, as the project is compiled with C++17, but we try to\n> > keep the public API compatible C++14. Condition the additional checks\n> > based on the C++ version.\n> > \n> > Signed-off-by: Laurent Pinchart \n> \n> I can't really say I have 'reviewed' this, however I like what it\n> promises, and the change itself looks small and straightforward.\n> \n> So because of unknown template magic, (that I could go read up on, but\n> I'm not going to right now), I'll just add this. I think it should go\n> in.\n\n:-)\n\nI went through the process of debugging this with Paul, the knowledge we\ngained should be shared. Let me give it a try.\n\nThe original error message from gcc is\n\n../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:\n../../include/libcamera/base/bound_method.h:143:4: required from here\n../../include/libcamera/base/bound_method.h:146:37: error: must use ‘.*’ or ‘->*’ to call pointer-to-member function in ‘((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_) (...)’\n 146 | return func_(args...);\n | ~~~~~^~~~~~~~~\n\nLet's focus on the error line first. The issue occurs at\n\n../../include/libcamera/base/bound_method.h:146:37\n\nand the error message is\n\nerror: must use ‘.*’ or ‘->*’ to call pointer-to-member function in [...]\n\nLet's look at the code on line 146:\n\ntemplate\nclass BoundMethodFunctor : public BoundMethodArgs\n{\npublic:\n[...]\n\tR activate(Args... args, bool deleteMethod = false) override\n\t{\n\t\tif (!this->object_)\n->\t\t\treturn func_(args...);\n\n\t\tauto pack = std::make_shared(args...);\n\t\tbool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n\t\treturn sync ? pack->returnValue() : R();\n\t}\n[...]\n};\n\nThe compiler complains that `func_(args...)` is not a valid way to call\na pointer-to-member function. That's right, a pointer-to-member needs to\nbe called on a particular object instance, such as in\nBoundMethodMember::activate():\n\n\treturn (obj->*func_)(args...);\n\nBut why are we reaching BoundMethodFunctor::activate() in the first\nplace if func_ is a pointer-to-member, as BoundMethodMember should be\nused in that case ? The answer lies in the rest of the error message:\n\n[...] in ‘((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_ (...)’, e.g. ‘(... ->* ((libcamera::BoundMethodFunctor*)this)->libcamera::BoundMethodFunctor::func_) (...)’\n\nLet's strip the libcamera:: namespace qualifier to make it easier to\nread:\n\n[...] in ‘((BoundMethodFunctor*)this)->BoundMethodFunctor::func_ (...)’ [...]\n\nThis means\n\n[...] in ‘((Type *)this)->Type::func_ (...)’ [...]\n\nwith Type being\n\nBoundMethodFunctor\n\nThat's already more readable, and indeed, the code tries to call\nfunc_(), which is a member variable of the BoundMethodFunctor class.\n\nSo why does the compiler pick BoundMethodFunctor if func_ is a\npointer-to-member ? Let's look at the error message from clang, which\nbrings more information:\n\n ../../include/libcamera/base/bound_method.h:146:11: error: called object type 'void (SignalTest::*)(int)' is not a function or function pointer\n return func_(args...);\n ^~~~~\n ../../include/libcamera/base/bound_method.h:137:2: note: in instantiation of member function 'libcamera::BoundMethodFunctor::activate' requested here\n BoundMethodFunctor(T *obj, Object *object, Func func,\n ^\n ../../include/libcamera/base/signal.h:80:27: note: in instantiation of member function 'libcamera::BoundMethodFunctor::BoundMethodFunctor' requested here\n SignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n ^\n ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect' requested here\n signalVoid_.connect(this, &SignalTest::slotInteger1);\n\nWe can see that BoundMethodFunctor is used in Signal::connect(), on line\n80 of signal.h (that information wasn't provided by gcc). The code reads\n\n\ttemplate::value> * = nullptr>\n\tvoid connect(T *obj, Func func)\n\t{\n\t\tSignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n\t}\n\nWe also have another version of connect, for pointer-to-member:\n\n\ttemplate::value> * = nullptr>\n\tvoid connect(T *obj, R (T::*func)(Args...))\n\t{\n\t\tSignalBase::connect(new BoundMethodMember(obj, nullptr, func));\n\t}\n\nSo why does the compiler pick the first one ? When performing overload\nresolution (multiple functions with the same name), the compiler will\ntry to match the arguments passed to the function with the arguments\nexpected by each overload. Here we see that both overloads have the same\nnumber of arguments, with a different type for the second argument. As\nthe functions are function templates, the compiler will then try to\nperform template argument resolution. The caller reads\n\n\t\tsignalVoid_.connect(this, &SignalTest::slotInteger1);\n\nwith \"this\" being an instance of SignalTest, and slotInteger1 being a\nmember function declared as\n\n\tvoid slotInteger1(int value);\n\nT is thus resolved to SignalTest, that's easy. For the second argument,\n`R (T::*func)(Args...)`, R is resolved to void, T matches the previous\nresolution (as slotInteger1 is a member of SignalTest). Note that Args\nis not a template argument of the function, but of the Signal class\nitself:\n\ntemplate\nclass Signal : public SignalBase\n{\n[...]\n};\n\nLet's now go back to the error message from clang, and in particular to\n\n ../../test/signal.cpp:110:15: note: in instantiation of function template specialization 'libcamera::Signal<>::connect' requested here\n\nThis shows that Signal is instantiated as Signal<>, which is right, in\ntest/signal.cpp we have\n\n\tSignal<> signalVoid_;\n\nArgs... thus resolves to an empty list, but that doesn't match the\narguments to slotInteger1 ! The compiler thus fails to perform template\nargument resolution for the Signal::connect() overload that is meant to\nhandle pointer-to-member arguments, and thus ignores it. What does it\nthen do ? It picks another candidate for the overload resolution:\n\n\ttemplate::value> * = nullptr>\n\tvoid connect(T *obj, Func func)\n\t{\n\t\tSignalBase::connect(new BoundMethodFunctor(obj, nullptr, func));\n\t}\n\nThere again it tries to perform template argument resolution. T matches\nSignalTest as before, and Func happily matches (SignalTest::*)(int), a\npointer-to-member, even though the code has been written to expect a\nfunctor (an object that can be directly called, such as a lambda, a\nstd::function or an instance of any class that has an operator()). The\noverload is resolved, and the compiler continues by instantiating a\nBoundMethodFunctor as instructed by the function. That's where\ncompilation fails, as BoundMethodFunctor tries to call the functor, and\na pointer-to-member can't be called that way.\n\nWhat does this teach us?\n\n- The solution was in the error message :-) Even without clang, gcc\n indicated\n\n../../include/libcamera/base/bound_method.h: In instantiation of ‘R libcamera::BoundMethodFunctor::activate(Args ..., bool) [with T = SignalTest; R = void; Func = void (SignalTest::*)(int); Args = {}]’:\n\n which shows a mismatch between the arguments to Func (int) and the\n Args (empty list {}). Disection error messages is scary, but if you\n split them across multiple lines and drop full namespace qualifiers,\n they become more readable.\n\n- clang still gives better error messages than gcc, not so much related\n to the error itself in this case, but there's more information about\n the callers for error related to template argument resolution.\n\n- This could have been avoided by proper usage of std::enable_if<> to\n ensure that the template arguments match the expected types.\n\n> Acked-by: Kieran Bingham \n> \n> > ---\n> > include/libcamera/base/signal.h | 12 ++++++++++--\n> > 1 file changed, 10 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/signal.h b/include/libcamera/base/signal.h\n> > index efb591bc5073..841e4b4ca15c 100644\n> > --- a/include/libcamera/base/signal.h\n> > +++ b/include/libcamera/base/signal.h\n> > @@ -63,7 +63,11 @@ public:\n> > \n> > #ifndef __DOXYGEN__\n> > template > - std::enable_if_t::value> * = nullptr>\n> > + std::enable_if_t::value\n> > +#if __cplusplus >= 201703L\n> > + && std::is_invocable_v\n> > +#endif\n> > + > * = nullptr>\n> > void connect(T *obj, Func func, ConnectionType type = ConnectionTypeAuto)\n> > {\n> > Object *object = static_cast(obj);\n> > @@ -71,7 +75,11 @@ public:\n> > }\n> > \n> > template > - std::enable_if_t::value> * = nullptr>\n> > + std::enable_if_t::value\n> > +#if __cplusplus >= 201703L\n> > + && std::is_invocable_v\n> > +#endif\n> > + > * = nullptr>\n> > #else\n> > template\n> > #endif","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 161D8C0DA4\n\tfor ;\n\tWed, 31 Aug 2022 10:58:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5079861FC0;\n\tWed, 31 Aug 2022 12:58:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EED161F9F\n\tfor ;\n\tWed, 31 Aug 2022 12:58:04 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2AACE481;\n\tWed, 31 Aug 2022 12:58:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661943485;\n\tbh=6vpq6IfyvGHEZRoOxyIJLGOBodSoVSl/zhr5FjU7H54=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4G2MwlD4XUQMJHmczhV6qNig2nqJEGvzmStYkrtRvdJbunlLkb69NoQEa/lydVm3R\n\tBRVVWWcEPwJOO/gfsrUikYxfElPLH9vqA3h2KOyGX6YJ2UJNQOeSl7AUBoYFDYotoA\n\tzoIhNjz6fbEdciPWcbtGgRSi5WvD3qNGlxBjsqgfnJgKNM02B+Q/smjQcA2OFZdiWW\n\tQWra4iDikLclyg1Q2gvnatVuf283KCrXK8kl0G0JCSDlCTsuTqzb5rQapb01nBTOZH\n\t5DgaV3bh35RStoQeb05vHXNyf8yY3Icpb2L4tfxZ9W6xeHQrBLQSFgTllHWDK5RoCk\n\tmrgkaKVXRgKdQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661943483;\n\tbh=6vpq6IfyvGHEZRoOxyIJLGOBodSoVSl/zhr5FjU7H54=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uBBRSzBsDIn0OmLWzpVj/RIQby1gu6ESE+hhaBS6HGn9dzjHYFRv5WyG9u0cU/V+A\n\taq+kP22TfKNa98Imp34SdUIfgySxlBCzItROxjNEQQR0tARXBJzbU95GcWUOCO3BNA\n\tmKulXEEpwg7C7PEJNruQniVBFRothL65FAs9vYfc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uBBRSzBs\"; dkim-atps=neutral","Date":"Wed, 31 Aug 2022 13:57:51 +0300","To":"Kieran Bingham ","Message-ID":"","References":"<20220830030615.29089-1-laurent.pinchart@ideasonboard.com>\n\t<166194105211.15972.2186307564879666027@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<166194105211.15972.2186307564879666027@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: base: signal: Disable\n\tconnect() for functor if args mismatch","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]