[{"id":19183,"web_url":"https://patchwork.libcamera.org/comment/19183/","msgid":"","date":"2021-08-30T16:36:25","subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThanks for the patch\n\nOn 8/27/21 8:08 AM, Laurent Pinchart wrote:\n> The BoundMethodArgs specialization for the void return type is only\n> needed to avoid accessing the ret_ member variable that is lacking from\n> the corresponding BoundMethodPack specialization. As the member variable\n> is only accessed in a single function, instead of specializing the whole\n> class we can use SFINAE to select between two different implementations\n> of the function.\n>\n> Signed-off-by: Laurent Pinchart \n\nLooks good overall to me, however the diff generated is confusing? I'll \napply it locally and re-read bound_method.h for my understanding.\n\nReviewed-by: Umang Jain \n\n> ---\n> include/libcamera/base/bound_method.h | 34 +++++++--------------------\n> 1 file changed, 8 insertions(+), 26 deletions(-)\n>\n> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> index 9c212f1ecc3a..76ce8017e721 100644\n> --- a/include/libcamera/base/bound_method.h\n> +++ b/include/libcamera/base/bound_method.h\n> @@ -98,35 +98,17 @@ public:\n> \tusing PackType = BoundMethodPack;\n> \n> private:\n> -\ttemplate\n> -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> +\ttemplate\n> +\tstd::enable_if_t::value, void>\n> +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> \t{\n> \t\tPackType *args = static_cast(pack);\n> \t\targs->ret_ = invoke(std::get(args->args_)...);\n> \t}\n> \n> -public:\n> -\tBoundMethodArgs(void *obj, Object *object, ConnectionType type)\n> -\t\t: BoundMethodBase(obj, object, type) {}\n> -\n> -\tvoid invokePack(BoundMethodPackBase *pack) override\n> -\t{\n> -\t\tinvokePack(pack, std::make_index_sequence{});\n> -\t}\n> -\n> -\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> -\tvirtual R invoke(Args... args) = 0;\n> -};\n> -\n> -template\n> -class BoundMethodArgs : public BoundMethodBase\n> -{\n> -public:\n> -\tusing PackType = BoundMethodPack;\n> -\n> -private:\n> -\ttemplate\n> -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> +\ttemplate\n> +\tstd::enable_if_t::value, void>\n> +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> \t{\n> \t\t/* args is effectively unused when the sequence I is empty. */\n> \t\tPackType *args [[gnu::unused]] = static_cast(pack);\n> @@ -142,8 +124,8 @@ public:\n> \t\tinvokePack(pack, std::make_index_sequence{});\n> \t}\n> \n> -\tvirtual void activate(Args... args, bool deleteMethod = false) = 0;\n> -\tvirtual void invoke(Args... args) = 0;\n> +\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> +\tvirtual R invoke(Args... args) = 0;\n> };\n> \n> template","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 2A00CBDC71\n\tfor ;\n\tMon, 30 Aug 2021 16:36:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E66F6916C;\n\tMon, 30 Aug 2021 18:36:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5669D60258\n\tfor ;\n\tMon, 30 Aug 2021 18:36:30 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.222])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64C9C5A7;\n\tMon, 30 Aug 2021 18:36:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j+n4d/JY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630341389;\n\tbh=3WIUONBBxlZcO+CS1+N0UAexLuQ897hkvgmCPY4E9pI=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=j+n4d/JYy3G/1amUfZ67Gqvchz6kAdr7EXf8DWjHtbkoDyA5RA0NmcKfqCfzzXZlo\n\tSFPn8+7roKcJzytNDBxoHEEApOVcpfTwUkS7paqQSkGqfU9u0a/8b+vIKPyxn9IJvS\n\tZsjxe3azFVFuYBCMg4Mcd028pLTxrpQcDTzksnkU=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210827023829.5871-1-laurent.pinchart@ideasonboard.com>\n\t<20210827023829.5871-4-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 30 Aug 2021 22:06:25 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827023829.5871-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19203,"web_url":"https://patchwork.libcamera.org/comment/19203/","msgid":"","date":"2021-08-31T05:43:12","subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/27/21 8:08 AM, Laurent Pinchart wrote:\n> The BoundMethodArgs specialization for the void return type is only\n> needed to avoid accessing the ret_ member variable that is lacking from\n> the corresponding BoundMethodPack specialization. As the member variable\n> is only accessed in a single function, instead of specializing the whole\n> class we can use SFINAE to select between two different implementations\n> of the function.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/bound_method.h | 34 +++++++--------------------\n> 1 file changed, 8 insertions(+), 26 deletions(-)\n>\n> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> index 9c212f1ecc3a..76ce8017e721 100644\n> --- a/include/libcamera/base/bound_method.h\n> +++ b/include/libcamera/base/bound_method.h\n> @@ -98,35 +98,17 @@ public:\n> \tusing PackType = BoundMethodPack;\n> \n> private:\n> -\ttemplate\n> -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> +\ttemplate\n> +\tstd::enable_if_t::value, void>\n> +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> \t{\n> \t\tPackType *args = static_cast(pack);\n> \t\targs->ret_ = invoke(std::get(args->args_)...);\n> \t}\n> \n> -public:\n> -\tBoundMethodArgs(void *obj, Object *object, ConnectionType type)\n> -\t\t: BoundMethodBase(obj, object, type) {}\n> -\n> -\tvoid invokePack(BoundMethodPackBase *pack) override\n> -\t{\n> -\t\tinvokePack(pack, std::make_index_sequence{});\n> -\t}\n> -\n> -\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> -\tvirtual R invoke(Args... args) = 0;\n> -};\n> -\n> -template\n> -class BoundMethodArgs : public BoundMethodBase\n> -{\n> -public:\n> -\tusing PackType = BoundMethodPack;\n> -\n> -private:\n> -\ttemplate\n> -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> +\ttemplate\n\n\nAs you have explained to me the workaround of T= R on IRC, that \nstd::enable_if works on function template argument typename rather on \nclass template argument, do you mind capturing this as a comment here or \nin the commit message while pushing?\n\n> +\tstd::enable_if_t::value, void>\n> +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> \t{\n> \t\t/* args is effectively unused when the sequence I is empty. */\n> \t\tPackType *args [[gnu::unused]] = static_cast(pack);\n> @@ -142,8 +124,8 @@ public:\n> \t\tinvokePack(pack, std::make_index_sequence{});\n> \t}\n> \n> -\tvirtual void activate(Args... args, bool deleteMethod = false) = 0;\n> -\tvirtual void invoke(Args... args) = 0;\n> +\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> +\tvirtual R invoke(Args... args) = 0;\n> };\n> \n> template","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 233D6BDC71\n\tfor ;\n\tTue, 31 Aug 2021 05:43:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 563F36916A;\n\tTue, 31 Aug 2021 07:43:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B119860137\n\tfor ;\n\tTue, 31 Aug 2021 07:43:19 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81A79323;\n\tTue, 31 Aug 2021 07:43:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fHEVgWRx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630388599;\n\tbh=kXi5GEIWQXqkEScyPPCwm9iBmB4CGGjQZCKB5N+njB0=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fHEVgWRxVXJ/o6ST7VuNvBlOyNfb4B3tKDjKXP2/w6OkugxtPY9TtiqBfohUdfhg3\n\tqvZY6QZ1RPGGZ5dFjRl/VJ5F7cR4qPmUCbC78zGb1sNeiymtSn625iujSFhrDFgc9f\n\tdJ1t6jiqOc2IftrqVy+/MdmrpaiIQX1AsPVGe+jE=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210827023829.5871-1-laurent.pinchart@ideasonboard.com>\n\t<20210827023829.5871-4-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 31 Aug 2021 11:13:12 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210827023829.5871-4-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19207,"web_url":"https://patchwork.libcamera.org/comment/19207/","msgid":"","date":"2021-08-31T09:28:37","subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Aug 31, 2021 at 11:13:12AM +0530, Umang Jain wrote:\n> On 8/27/21 8:08 AM, Laurent Pinchart wrote:\n> > The BoundMethodArgs specialization for the void return type is only\n> > needed to avoid accessing the ret_ member variable that is lacking from\n> > the corresponding BoundMethodPack specialization. As the member variable\n> > is only accessed in a single function, instead of specializing the whole\n> > class we can use SFINAE to select between two different implementations\n> > of the function.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/bound_method.h | 34 +++++++--------------------\n> > 1 file changed, 8 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> > index 9c212f1ecc3a..76ce8017e721 100644\n> > --- a/include/libcamera/base/bound_method.h\n> > +++ b/include/libcamera/base/bound_method.h\n> > @@ -98,35 +98,17 @@ public:\n> > \tusing PackType = BoundMethodPack;\n> > \n> > private:\n> > -\ttemplate\n> > -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> > +\ttemplate\n> > +\tstd::enable_if_t::value, void>\n> > +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> > \t{\n> > \t\tPackType *args = static_cast(pack);\n> > \t\targs->ret_ = invoke(std::get(args->args_)...);\n> > \t}\n> > \n> > -public:\n> > -\tBoundMethodArgs(void *obj, Object *object, ConnectionType type)\n> > -\t\t: BoundMethodBase(obj, object, type) {}\n> > -\n> > -\tvoid invokePack(BoundMethodPackBase *pack) override\n> > -\t{\n> > -\t\tinvokePack(pack, std::make_index_sequence{});\n> > -\t}\n> > -\n> > -\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> > -\tvirtual R invoke(Args... args) = 0;\n> > -};\n> > -\n> > -template\n> > -class BoundMethodArgs : public BoundMethodBase\n> > -{\n> > -public:\n> > -\tusing PackType = BoundMethodPack;\n> > -\n> > -private:\n> > -\ttemplate\n> > -\tvoid invokePack(BoundMethodPackBase *pack, std::index_sequence)\n> > +\ttemplate\n> \n> \n> As you have explained to me the workaround of T= R on IRC, that \n> std::enable_if works on function template argument typename rather on \n> class template argument, do you mind capturing this as a comment here or \n> in the commit message while pushing?\n\nI'll add this to the commit messge:\n\nSFINAE can only depend on the function template parameters, not the\nparameters of the class template in which the function is defined:\n\n\"Only the failures in the types and expressions in the immediate context\nof the function type or its template parameter types are SFINAE errors.\"\n\nWe thus can't use the type R in an std::enable_if expression for the\ninvokePack() function. To work around this, we have to add a type T to\nthe function template definition, which defaults to R, and use T with\nstd::enable_if.\n\n> > +\tstd::enable_if_t::value, void>\n> > +\tinvokePack(BoundMethodPackBase *pack, std::index_sequence)\n> > \t{\n> > \t\t/* args is effectively unused when the sequence I is empty. */\n> > \t\tPackType *args [[gnu::unused]] = static_cast(pack);\n> > @@ -142,8 +124,8 @@ public:\n> > \t\tinvokePack(pack, std::make_index_sequence{});\n> > \t}\n> > \n> > -\tvirtual void activate(Args... args, bool deleteMethod = false) = 0;\n> > -\tvirtual void invoke(Args... args) = 0;\n> > +\tvirtual R activate(Args... args, bool deleteMethod = false) = 0;\n> > +\tvirtual R invoke(Args... args) = 0;\n> > };\n> > \n> > template","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 96CD7BDC71\n\tfor ;\n\tTue, 31 Aug 2021 09:28:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2142A6916A;\n\tTue, 31 Aug 2021 11:28:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62AFF60288\n\tfor ;\n\tTue, 31 Aug 2021 11:28:52 +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 CB6F03D7;\n\tTue, 31 Aug 2021 11:28:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HDsgEHnE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630402132;\n\tbh=+pYA13Mcg7+rhGDmH4DlLShd2uGYoFwfcsopebkkMz0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HDsgEHnEwZdmWWhSL0htwp3apoiH3JmQ6eMxwxHo1fvkv0PtRez9HBXUjBvY7Yd7L\n\tbJ5S5zh+gqEs3qeCVpUxQoZNhBb+ikMCvyftafvembufFC9kQEtZbfRJheVxz1Qn30\n\tuNepH6tzn71hvHwRIc7iQIe8bDxGTfzW+X37LLSM=","Date":"Tue, 31 Aug 2021 12:28:37 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210827023829.5871-1-laurent.pinchart@ideasonboard.com>\n\t<20210827023829.5871-4-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/6] libcamera: base: bound_method:\n\tRemove BoundMethodArgs specialization","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]