[{"id":19182,"web_url":"https://patchwork.libcamera.org/comment/19182/","msgid":"","date":"2021-08-30T16:28:59","subject":"Re: [libcamera-devel] [PATCH v1 2/6] libcamera: base: bound_method:\n\tRemove BoundMethodMember 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 BoundMethodMember 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. By adding a\n> BoundMethodPack::returnValue() function to read the member variable, we\n> can remove the complete BoundMethodMember specialization.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/bound_method.h | 46 ++++++---------------------\n> 1 file changed, 10 insertions(+), 36 deletions(-)\n\n\nOk Let's see, my review attempt #3 to understand these patches. There is \nquite a level of inheritance of classes going on here.. so I hope I \ndon't get lost.\n\n>\n> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> index 282f9b58ab60..9c212f1ecc3a 100644\n> --- a/include/libcamera/base/bound_method.h\n> +++ b/include/libcamera/base/bound_method.h\n> @@ -38,6 +38,11 @@ public:\n> \t{\n> \t}\n> \n> +\tR returnValue()\n> +\t{\n> +\t\treturn ret_;\n> +\t}\n> +\nMake sense.\n> \tstd::tuple...> args_;\n> \tR ret_;\n> };\n> @@ -51,6 +56,10 @@ public:\n> \t{\n> \t}\n> \n> +\tvoid returnValue()\n> +\t{\n> +\t}\n> +\n\n\nMakes sense\n\n> \tstd::tuple...> args_;\n> };\n> \n> @@ -160,7 +169,7 @@ public:\n> \n> \t\tauto pack = std::make_shared(args...);\n> \t\tbool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> -\t\treturn sync ? pack->ret_ : R();\n> +\t\treturn sync ? pack->returnValue() : R();\n\n\nYep\n\n> \t}\n> \n> \tR invoke(Args... args) override\n> @@ -173,41 +182,6 @@ private:\n> \tR (T::*func_)(Args...);\n> };\n> \n> -template\n> -class BoundMethodMember : public BoundMethodArgs\n\nAh nice. This is nuked.\n\nI was going to write that we probably need to nuke BoundMethodArgs as no class inherits that now.\n\n...But I already see that happening in 3/6, so slowly I am getting the \nessence of it yay!\n\nReviewed-by: Umang Jain \n\n> -{\n> -public:\n> -\tusing PackType = typename BoundMethodArgs::PackType;\n> -\n> -\tBoundMethodMember(T *obj, Object *object, void (T::*func)(Args...),\n> -\t\t\t ConnectionType type = ConnectionTypeAuto)\n> -\t\t: BoundMethodArgs(obj, object, type), func_(func)\n> -\t{\n> -\t}\n> -\n> -\tbool match(void (T::*func)(Args...)) const { return func == func_; }\n> -\n> -\tvoid activate(Args... args, bool deleteMethod = false) override\n> -\t{\n> -\t\tif (!this->object_) {\n> -\t\t\tT *obj = static_cast(this->obj_);\n> -\t\t\treturn (obj->*func_)(args...);\n> -\t\t}\n> -\n> -\t\tauto pack = std::make_shared(args...);\n> -\t\tBoundMethodBase::activatePack(pack, deleteMethod);\n> -\t}\n> -\n> -\tvoid invoke(Args... args) override\n> -\t{\n> -\t\tT *obj = static_cast(this->obj_);\n> -\t\treturn (obj->*func_)(args...);\n> -\t}\n> -\n> -private:\n> -\tvoid (T::*func_)(Args...);\n> -};\n> -\n> template\n> class BoundMethodStatic : public BoundMethodArgs\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 9C5B9BD87D\n\tfor ;\n\tMon, 30 Aug 2021 16:29:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D0436916A;\n\tMon, 30 Aug 2021 18:29:07 +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 4C4DC60258\n\tfor ;\n\tMon, 30 Aug 2021 18:29:05 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.222])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A6C05A7;\n\tMon, 30 Aug 2021 18:29:04 +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=\"FcBA8H4U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630340944;\n\tbh=UrXGXG8BzVJnysv7DydtuxHJks4c4fazmGxPhff+OoE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=FcBA8H4UUYE+73JTCkz7LCe1vGswh2yUjIv9bduMJP8vnpk0eJu/WeSXfCZtIvsxq\n\taR+xjEkg/1Om8tRKQvDp/0tcx1Q7eZmNuNymwa51YT4Y59JnlhrK48n971KH1ofgyo\n\tRzGI/6GI4NiSITZPFlEL8o8fVgGXGHBxve8KU3Kc=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210827023829.5871-1-laurent.pinchart@ideasonboard.com>\n\t<20210827023829.5871-3-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 30 Aug 2021 21:58:59 +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-3-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 2/6] libcamera: base: bound_method:\n\tRemove BoundMethodMember 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\" "}}]