[{"id":34936,"web_url":"https://patchwork.libcamera.org/comment/34936/","msgid":"<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>","date":"2025-07-18T13:54:49","subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-06-19 10:32:18)\n> Instead of copying, just move the returned value when the call is made\n> through an argument pack. This enables, e.g. `Object::invokeMethod()`\n> to be usable with functions returning types, such as`std::unique_ptr`,\n> that have no copy ctor/assignment. Since there are no other users of\n> the argument pack object, this is safe to do. Reference return types\n> are not supported, so a simple `std::move()` is sufficient.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=273#c1\n> Signed-off-by: Barnabás Pőcze \n> Reviewed-by: Laurent Pinchart \n\nIs there any ABI breakage on this that I need to consider?\n\nOtherwise \n\n\nReviewed-by: Kieran Bingham \n\n> ---\n> changes in v2:\n> * add `[[maybe_unused]]` for gcc 9\n> * review comments\n> \n> v1: https://patchwork.libcamera.org/patch/23598/\n> ---\n> include/libcamera/base/bound_method.h | 21 ++++++++-------------\n> test/object-invoke.cpp | 17 +++++++++++++++++\n> 2 files changed, 25 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> index 507c320d3..e14614bc4 100644\n> --- a/include/libcamera/base/bound_method.h\n> +++ b/include/libcamera/base/bound_method.h\n> @@ -38,11 +38,6 @@ public:\n> {\n> }\n> \n> - R returnValue()\n> - {\n> - return ret_;\n> - }\n> -\n> std::tuple...> args_;\n> R ret_;\n> };\n> @@ -56,10 +51,6 @@ public:\n> {\n> }\n> \n> - void returnValue()\n> - {\n> - }\n> -\n> std::tuple...> args_;\n> };\n> \n> @@ -140,8 +131,10 @@ public:\n> return func_(args...);\n> \n> auto pack = std::make_shared(args...);\n> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> - return sync ? pack->returnValue() : R();\n> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> +\n> + if constexpr (!std::is_void_v)\n> + return sync ? std::move(pack->ret_) : R();\n> }\n> \n> R invoke(Args... args) override\n> @@ -175,8 +168,10 @@ public:\n> }\n> \n> auto pack = std::make_shared(args...);\n> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> - return sync ? pack->returnValue() : R();\n> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> +\n> + if constexpr (!std::is_void_v)\n> + return sync ? std::move(pack->ret_) : R();\n> }\n> \n> R invoke(Args... args) override\n> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n> index def1e61e4..ab4a15751 100644\n> --- a/test/object-invoke.cpp\n> +++ b/test/object-invoke.cpp\n> @@ -58,6 +58,19 @@ public:\n> return 42;\n> }\n> \n> + struct MoveOnly {\n> + MoveOnly() = default;\n> + MoveOnly(const MoveOnly &) = delete;\n> + MoveOnly &operator=(const MoveOnly &) = delete;\n> + MoveOnly(MoveOnly &&) = default;\n> + MoveOnly &operator=(MoveOnly &&) = default;\n> + };\n> +\n> + MoveOnly methodWithReturnMoveOnly()\n> + {\n> + return {};\n> + }\n> +\n> private:\n> Status status_;\n> int value_;\n> @@ -186,6 +199,10 @@ protected:\n> return TestFail;\n> }\n> \n> + /* Test invoking a method that returns type with no copy ctor/assignment. */\n> + object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly,\n> + ConnectionTypeBlocking);\n> +\n> return TestPass;\n> }\n> \n> --\n> 2.50.0","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 EA316BE175\n\tfor ;\n\tFri, 18 Jul 2025 13:54:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C785268FA4;\n\tFri, 18 Jul 2025 15:54:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 700D26150F\n\tfor ;\n\tFri, 18 Jul 2025 15:54:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A82AE59DF;\n\tFri, 18 Jul 2025 15:54:17 +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=\"fMZBJIiQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752846857;\n\tbh=R6RF7MZyEr3VmYv6LtGBDzTWD8F3mWT3t35X3WktdR4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=fMZBJIiQEr3TWx940/cK0PpEI/bF/mDwDYwghADOzgKIN6kW4hqGxaoNsXYOkllN0\n\tOSdp72WOoNr3+bubbMkKCZcr4axsrnla+JlNX7q9ZING0yUqqnXMk2PYibBbhskQbu\n\tYwXSZu7YXjruI/ix370Tk7UknVPGgOjF6jCAt5HQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>","References":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","From":"Kieran Bingham ","Cc":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 18 Jul 2025 14:54:49 +0100","Message-ID":"<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":34938,"web_url":"https://patchwork.libcamera.org/comment/34938/","msgid":"","date":"2025-07-18T14:01:20","subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 18. 15:54 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-06-19 10:32:18)\n>> Instead of copying, just move the returned value when the call is made\n>> through an argument pack. This enables, e.g. `Object::invokeMethod()`\n>> to be usable with functions returning types, such as`std::unique_ptr`,\n>> that have no copy ctor/assignment. Since there are no other users of\n>> the argument pack object, this is safe to do. Reference return types\n>> are not supported, so a simple `std::move()` is sufficient.\n>>\n>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=273#c1\n>> Signed-off-by: Barnabás Pőcze \n>> Reviewed-by: Laurent Pinchart \n> \n> Is there any ABI breakage on this that I need to consider?\n\nThere is an API change. It removes `returnValue()`, but I would still like\nto merge it because no one should be using these types directly (and I would\nbe surprised if anyone did). No effect on ABI. Thoughts?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Otherwise\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n>> ---\n>> changes in v2:\n>> * add `[[maybe_unused]]` for gcc 9\n>> * review comments\n>>\n>> v1: https://patchwork.libcamera.org/patch/23598/\n>> ---\n>> include/libcamera/base/bound_method.h | 21 ++++++++-------------\n>> test/object-invoke.cpp | 17 +++++++++++++++++\n>> 2 files changed, 25 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n>> index 507c320d3..e14614bc4 100644\n>> --- a/include/libcamera/base/bound_method.h\n>> +++ b/include/libcamera/base/bound_method.h\n>> @@ -38,11 +38,6 @@ public:\n>> {\n>> }\n>>\n>> - R returnValue()\n>> - {\n>> - return ret_;\n>> - }\n>> -\n>> std::tuple...> args_;\n>> R ret_;\n>> };\n>> @@ -56,10 +51,6 @@ public:\n>> {\n>> }\n>>\n>> - void returnValue()\n>> - {\n>> - }\n>> -\n>> std::tuple...> args_;\n>> };\n>>\n>> @@ -140,8 +131,10 @@ public:\n>> return func_(args...);\n>>\n>> auto pack = std::make_shared(args...);\n>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>> - return sync ? pack->returnValue() : R();\n>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>> +\n>> + if constexpr (!std::is_void_v)\n>> + return sync ? std::move(pack->ret_) : R();\n>> }\n>>\n>> R invoke(Args... args) override\n>> @@ -175,8 +168,10 @@ public:\n>> }\n>>\n>> auto pack = std::make_shared(args...);\n>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>> - return sync ? pack->returnValue() : R();\n>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>> +\n>> + if constexpr (!std::is_void_v)\n>> + return sync ? std::move(pack->ret_) : R();\n>> }\n>>\n>> R invoke(Args... args) override\n>> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n>> index def1e61e4..ab4a15751 100644\n>> --- a/test/object-invoke.cpp\n>> +++ b/test/object-invoke.cpp\n>> @@ -58,6 +58,19 @@ public:\n>> return 42;\n>> }\n>>\n>> + struct MoveOnly {\n>> + MoveOnly() = default;\n>> + MoveOnly(const MoveOnly &) = delete;\n>> + MoveOnly &operator=(const MoveOnly &) = delete;\n>> + MoveOnly(MoveOnly &&) = default;\n>> + MoveOnly &operator=(MoveOnly &&) = default;\n>> + };\n>> +\n>> + MoveOnly methodWithReturnMoveOnly()\n>> + {\n>> + return {};\n>> + }\n>> +\n>> private:\n>> Status status_;\n>> int value_;\n>> @@ -186,6 +199,10 @@ protected:\n>> return TestFail;\n>> }\n>>\n>> + /* Test invoking a method that returns type with no copy ctor/assignment. */\n>> + object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly,\n>> + ConnectionTypeBlocking);\n>> +\n>> return TestPass;\n>> }\n>>\n>> --\n>> 2.50.0","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 EAA13BE175\n\tfor ;\n\tFri, 18 Jul 2025 14:01:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A0B868FA7;\n\tFri, 18 Jul 2025 16:01:29 +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 66ADA6150F\n\tfor ;\n\tFri, 18 Jul 2025 16:01:27 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 706EB59DF;\n\tFri, 18 Jul 2025 16:00:52 +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=\"vCI7v+cM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752847252;\n\tbh=RNpt8Pwzf+InTjND1DhnFf477+CrSzSSAhbk2kPU3L8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vCI7v+cMpt1uZiP6E2EQ8hFtWJgDC9FaD35+JUWhaqucm29meEeXmpUwUA1KL2Zvz\n\t8oMvcHJ+UGp/zkx+502VCauin0eX4NeMC5/pRDPyB5gTqKVz8WOamLmJrvPXrvO/Fj\n\tzpSKZpFQ/pIBJ+wlm5ksN8FMvIG36wI4+bc5lAco=","Message-ID":"","Date":"Fri, 18 Jul 2025 16:01:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ","References":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>\n\t<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34942,"web_url":"https://patchwork.libcamera.org/comment/34942/","msgid":"<175284767383.560048.6382746607051981355@ping.linuxembedded.co.uk>","date":"2025-07-18T14:07:53","subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-07-18 15:01:20)\n> Hi\n> \n> 2025. 07. 18. 15:54 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-06-19 10:32:18)\n> >> Instead of copying, just move the returned value when the call is made\n> >> through an argument pack. This enables, e.g. `Object::invokeMethod()`\n> >> to be usable with functions returning types, such as`std::unique_ptr`,\n> >> that have no copy ctor/assignment. Since there are no other users of\n> >> the argument pack object, this is safe to do. Reference return types\n> >> are not supported, so a simple `std::move()` is sufficient.\n> >>\n> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=273#c1\n> >> Signed-off-by: Barnabás Pőcze \n> >> Reviewed-by: Laurent Pinchart \n> > \n> > Is there any ABI breakage on this that I need to consider?\n> \n> There is an API change. It removes `returnValue()`, but I would still like\n> to merge it because no one should be using these types directly (and I would\n> be surprised if anyone did). No effect on ABI. Thoughts?\n> \n\nIf it's helpful then I'd like it to be merged too.\n\nWhat's the result of running ./utils/abi-compat.sh ?\n\n--\nKieran\n\n\n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> > Otherwise\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> >> ---\n> >> changes in v2:\n> >> * add `[[maybe_unused]]` for gcc 9\n> >> * review comments\n> >>\n> >> v1: https://patchwork.libcamera.org/patch/23598/\n> >> ---\n> >> include/libcamera/base/bound_method.h | 21 ++++++++-------------\n> >> test/object-invoke.cpp | 17 +++++++++++++++++\n> >> 2 files changed, 25 insertions(+), 13 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> >> index 507c320d3..e14614bc4 100644\n> >> --- a/include/libcamera/base/bound_method.h\n> >> +++ b/include/libcamera/base/bound_method.h\n> >> @@ -38,11 +38,6 @@ public:\n> >> {\n> >> }\n> >>\n> >> - R returnValue()\n> >> - {\n> >> - return ret_;\n> >> - }\n> >> -\n> >> std::tuple...> args_;\n> >> R ret_;\n> >> };\n> >> @@ -56,10 +51,6 @@ public:\n> >> {\n> >> }\n> >>\n> >> - void returnValue()\n> >> - {\n> >> - }\n> >> -\n> >> std::tuple...> args_;\n> >> };\n> >>\n> >> @@ -140,8 +131,10 @@ public:\n> >> return func_(args...);\n> >>\n> >> auto pack = std::make_shared(args...);\n> >> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >> - return sync ? pack->returnValue() : R();\n> >> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >> +\n> >> + if constexpr (!std::is_void_v)\n> >> + return sync ? std::move(pack->ret_) : R();\n> >> }\n> >>\n> >> R invoke(Args... args) override\n> >> @@ -175,8 +168,10 @@ public:\n> >> }\n> >>\n> >> auto pack = std::make_shared(args...);\n> >> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >> - return sync ? pack->returnValue() : R();\n> >> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >> +\n> >> + if constexpr (!std::is_void_v)\n> >> + return sync ? std::move(pack->ret_) : R();\n> >> }\n> >>\n> >> R invoke(Args... args) override\n> >> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n> >> index def1e61e4..ab4a15751 100644\n> >> --- a/test/object-invoke.cpp\n> >> +++ b/test/object-invoke.cpp\n> >> @@ -58,6 +58,19 @@ public:\n> >> return 42;\n> >> }\n> >>\n> >> + struct MoveOnly {\n> >> + MoveOnly() = default;\n> >> + MoveOnly(const MoveOnly &) = delete;\n> >> + MoveOnly &operator=(const MoveOnly &) = delete;\n> >> + MoveOnly(MoveOnly &&) = default;\n> >> + MoveOnly &operator=(MoveOnly &&) = default;\n> >> + };\n> >> +\n> >> + MoveOnly methodWithReturnMoveOnly()\n> >> + {\n> >> + return {};\n> >> + }\n> >> +\n> >> private:\n> >> Status status_;\n> >> int value_;\n> >> @@ -186,6 +199,10 @@ protected:\n> >> return TestFail;\n> >> }\n> >>\n> >> + /* Test invoking a method that returns type with no copy ctor/assignment. */\n> >> + object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly,\n> >> + ConnectionTypeBlocking);\n> >> +\n> >> return TestPass;\n> >> }\n> >>\n> >> --\n> >> 2.50.0\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 41A40C3237\n\tfor ;\n\tFri, 18 Jul 2025 14:07:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4E2F68FAC;\n\tFri, 18 Jul 2025 16:07:57 +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 B35AC68FA4\n\tfor ;\n\tFri, 18 Jul 2025 16:07:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04A8E7F0;\n\tFri, 18 Jul 2025 16:07:21 +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=\"R7mHarAc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752847642;\n\tbh=+8FsgkLTxoGHg+hpGM0lv9FVn1dyxbAg6YzMNMgjdJQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=R7mHarAcT2BbKbSSHVOcpkh/xjLzq3pYy6SJd+ncwk+yVM8rWV7LeurnL8SRX4iEL\n\tC3+H9dhgmAsfiSe3k2x6vUTarzn5wliwsLEn7RU03yfMEDrxpJrLqx/BLKg1uDpxJ8\n\te+aInxCajziLUYldQ9C1gsDwKiwJifX48HDH7qHQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>\n\t<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","From":"Kieran Bingham ","Cc":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 18 Jul 2025 15:07:53 +0100","Message-ID":"<175284767383.560048.6382746607051981355@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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":34943,"web_url":"https://patchwork.libcamera.org/comment/34943/","msgid":"","date":"2025-07-18T14:25:58","subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 18. 16:07 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-07-18 15:01:20)\n>> Hi\n>>\n>> 2025. 07. 18. 15:54 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-06-19 10:32:18)\n>>>> Instead of copying, just move the returned value when the call is made\n>>>> through an argument pack. This enables, e.g. `Object::invokeMethod()`\n>>>> to be usable with functions returning types, such as`std::unique_ptr`,\n>>>> that have no copy ctor/assignment. Since there are no other users of\n>>>> the argument pack object, this is safe to do. Reference return types\n>>>> are not supported, so a simple `std::move()` is sufficient.\n>>>>\n>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=273#c1\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> Reviewed-by: Laurent Pinchart \n>>>\n>>> Is there any ABI breakage on this that I need to consider?\n>>\n>> There is an API change. It removes `returnValue()`, but I would still like\n>> to merge it because no one should be using these types directly (and I would\n>> be surprised if anyone did). No effect on ABI. Thoughts?\n>>\n> \n> If it's helpful then I'd like it to be merged too.\n> \n> What's the result of running ./utils/abi-compat.sh ?\n\nApparently it doesn't detect the change? It says 100% source and binary\ncompatibility, and it is not listed under \"Removed Symbols\". This was\nbetween v0.5.1 and a9c2dd05fa3783e2284c64c558122ceb632eab1b + this patch.\n\n\n> \n> --\n> Kieran\n> \n> \n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>>\n>>> Otherwise\n>>>\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> ---\n>>>> changes in v2:\n>>>> * add `[[maybe_unused]]` for gcc 9\n>>>> * review comments\n>>>>\n>>>> v1: https://patchwork.libcamera.org/patch/23598/\n>>>> ---\n>>>> include/libcamera/base/bound_method.h | 21 ++++++++-------------\n>>>> test/object-invoke.cpp | 17 +++++++++++++++++\n>>>> 2 files changed, 25 insertions(+), 13 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n>>>> index 507c320d3..e14614bc4 100644\n>>>> --- a/include/libcamera/base/bound_method.h\n>>>> +++ b/include/libcamera/base/bound_method.h\n>>>> @@ -38,11 +38,6 @@ public:\n>>>> {\n>>>> }\n>>>>\n>>>> - R returnValue()\n>>>> - {\n>>>> - return ret_;\n>>>> - }\n>>>> -\n>>>> std::tuple...> args_;\n>>>> R ret_;\n>>>> };\n>>>> @@ -56,10 +51,6 @@ public:\n>>>> {\n>>>> }\n>>>>\n>>>> - void returnValue()\n>>>> - {\n>>>> - }\n>>>> -\n>>>> std::tuple...> args_;\n>>>> };\n>>>>\n>>>> @@ -140,8 +131,10 @@ public:\n>>>> return func_(args...);\n>>>>\n>>>> auto pack = std::make_shared(args...);\n>>>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>>>> - return sync ? pack->returnValue() : R();\n>>>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>>>> +\n>>>> + if constexpr (!std::is_void_v)\n>>>> + return sync ? std::move(pack->ret_) : R();\n>>>> }\n>>>>\n>>>> R invoke(Args... args) override\n>>>> @@ -175,8 +168,10 @@ public:\n>>>> }\n>>>>\n>>>> auto pack = std::make_shared(args...);\n>>>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>>>> - return sync ? pack->returnValue() : R();\n>>>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n>>>> +\n>>>> + if constexpr (!std::is_void_v)\n>>>> + return sync ? std::move(pack->ret_) : R();\n>>>> }\n>>>>\n>>>> R invoke(Args... args) override\n>>>> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n>>>> index def1e61e4..ab4a15751 100644\n>>>> --- a/test/object-invoke.cpp\n>>>> +++ b/test/object-invoke.cpp\n>>>> @@ -58,6 +58,19 @@ public:\n>>>> return 42;\n>>>> }\n>>>>\n>>>> + struct MoveOnly {\n>>>> + MoveOnly() = default;\n>>>> + MoveOnly(const MoveOnly &) = delete;\n>>>> + MoveOnly &operator=(const MoveOnly &) = delete;\n>>>> + MoveOnly(MoveOnly &&) = default;\n>>>> + MoveOnly &operator=(MoveOnly &&) = default;\n>>>> + };\n>>>> +\n>>>> + MoveOnly methodWithReturnMoveOnly()\n>>>> + {\n>>>> + return {};\n>>>> + }\n>>>> +\n>>>> private:\n>>>> Status status_;\n>>>> int value_;\n>>>> @@ -186,6 +199,10 @@ protected:\n>>>> return TestFail;\n>>>> }\n>>>>\n>>>> + /* Test invoking a method that returns type with no copy ctor/assignment. */\n>>>> + object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly,\n>>>> + ConnectionTypeBlocking);\n>>>> +\n>>>> return TestPass;\n>>>> }\n>>>>\n>>>> --\n>>>> 2.50.0\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 34F37BE175\n\tfor ;\n\tFri, 18 Jul 2025 14:26:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC01368FA9;\n\tFri, 18 Jul 2025 16:26:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE57D6150F\n\tfor ;\n\tFri, 18 Jul 2025 16:26:02 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A740559E3;\n\tFri, 18 Jul 2025 16:25:27 +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=\"VIU+kovW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752848727;\n\tbh=A+bwwLK3d4JzW6fAA2nF9wH6zeSykU4p++fw1OuC+l0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VIU+kovWIV4nfegktRkqVs/wTCGdO1c5rZupy+fXyjV/0zPFj1EYq7LoPV7zi3mR4\n\tn0jpi5CsaQrlmypZWCexVtI3kHj0pvExbSLVlYuAWnHOWHPYAfah//AGotjppabebo\n\tKjvBp9Svo08hTNI2xLNwa5l0ZQcB47Bm2QK4J+ZQ=","Message-ID":"","Date":"Fri, 18 Jul 2025 16:25:58 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Laurent Pinchart ","References":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>\n\t<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>\n\t\n\t<175284767383.560048.6382746607051981355@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175284767383.560048.6382746607051981355@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34946,"web_url":"https://patchwork.libcamera.org/comment/34946/","msgid":"<175286145240.560048.15679481925900621680@ping.linuxembedded.co.uk>","date":"2025-07-18T17:57:32","subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-07-18 15:25:58)\n> Hi\n> \n> 2025. 07. 18. 16:07 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-07-18 15:01:20)\n> >> Hi\n> >>\n> >> 2025. 07. 18. 15:54 keltezéssel, Kieran Bingham írta:\n> >>> Quoting Barnabás Pőcze (2025-06-19 10:32:18)\n> >>>> Instead of copying, just move the returned value when the call is made\n> >>>> through an argument pack. This enables, e.g. `Object::invokeMethod()`\n> >>>> to be usable with functions returning types, such as`std::unique_ptr`,\n> >>>> that have no copy ctor/assignment. Since there are no other users of\n> >>>> the argument pack object, this is safe to do. Reference return types\n> >>>> are not supported, so a simple `std::move()` is sufficient.\n> >>>>\n> >>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=273#c1\n> >>>> Signed-off-by: Barnabás Pőcze \n> >>>> Reviewed-by: Laurent Pinchart \n> >>>\n> >>> Is there any ABI breakage on this that I need to consider?\n> >>\n> >> There is an API change. It removes `returnValue()`, but I would still like\n> >> to merge it because no one should be using these types directly (and I would\n> >> be surprised if anyone did). No effect on ABI. Thoughts?\n> >>\n> > \n> > If it's helpful then I'd like it to be merged too.\n> > \n> > What's the result of running ./utils/abi-compat.sh ?\n> \n> Apparently it doesn't detect the change? It says 100% source and binary\n> compatibility, and it is not listed under \"Removed Symbols\". This was\n> between v0.5.1 and a9c2dd05fa3783e2284c64c558122ceb632eab1b + this patch.\n\nabi-compat will only check the public symbols, so maybe this one wasn't\npublic ?\n\nAnyway - perhaps that means it's easy to merge, as both the tooling is\nfine - and you believe it shouldn't be used anyway.\n\nFeel free to push.\n\n--\nRegards\n\nKieran\n\n\n> \n> \n> > \n> > --\n> > Kieran\n> > \n> > \n> >>\n> >> Regards,\n> >> Barnabás Pőcze\n> >>\n> >>>\n> >>> Otherwise\n> >>>\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> ---\n> >>>> changes in v2:\n> >>>> * add `[[maybe_unused]]` for gcc 9\n> >>>> * review comments\n> >>>>\n> >>>> v1: https://patchwork.libcamera.org/patch/23598/\n> >>>> ---\n> >>>> include/libcamera/base/bound_method.h | 21 ++++++++-------------\n> >>>> test/object-invoke.cpp | 17 +++++++++++++++++\n> >>>> 2 files changed, 25 insertions(+), 13 deletions(-)\n> >>>>\n> >>>> diff --git a/include/libcamera/base/bound_method.h b/include/libcamera/base/bound_method.h\n> >>>> index 507c320d3..e14614bc4 100644\n> >>>> --- a/include/libcamera/base/bound_method.h\n> >>>> +++ b/include/libcamera/base/bound_method.h\n> >>>> @@ -38,11 +38,6 @@ public:\n> >>>> {\n> >>>> }\n> >>>>\n> >>>> - R returnValue()\n> >>>> - {\n> >>>> - return ret_;\n> >>>> - }\n> >>>> -\n> >>>> std::tuple...> args_;\n> >>>> R ret_;\n> >>>> };\n> >>>> @@ -56,10 +51,6 @@ public:\n> >>>> {\n> >>>> }\n> >>>>\n> >>>> - void returnValue()\n> >>>> - {\n> >>>> - }\n> >>>> -\n> >>>> std::tuple...> args_;\n> >>>> };\n> >>>>\n> >>>> @@ -140,8 +131,10 @@ public:\n> >>>> return func_(args...);\n> >>>>\n> >>>> auto pack = std::make_shared(args...);\n> >>>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >>>> - return sync ? pack->returnValue() : R();\n> >>>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >>>> +\n> >>>> + if constexpr (!std::is_void_v)\n> >>>> + return sync ? std::move(pack->ret_) : R();\n> >>>> }\n> >>>>\n> >>>> R invoke(Args... args) override\n> >>>> @@ -175,8 +168,10 @@ public:\n> >>>> }\n> >>>>\n> >>>> auto pack = std::make_shared(args...);\n> >>>> - bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >>>> - return sync ? pack->returnValue() : R();\n> >>>> + [[maybe_unused]] bool sync = BoundMethodBase::activatePack(pack, deleteMethod);\n> >>>> +\n> >>>> + if constexpr (!std::is_void_v)\n> >>>> + return sync ? std::move(pack->ret_) : R();\n> >>>> }\n> >>>>\n> >>>> R invoke(Args... args) override\n> >>>> diff --git a/test/object-invoke.cpp b/test/object-invoke.cpp\n> >>>> index def1e61e4..ab4a15751 100644\n> >>>> --- a/test/object-invoke.cpp\n> >>>> +++ b/test/object-invoke.cpp\n> >>>> @@ -58,6 +58,19 @@ public:\n> >>>> return 42;\n> >>>> }\n> >>>>\n> >>>> + struct MoveOnly {\n> >>>> + MoveOnly() = default;\n> >>>> + MoveOnly(const MoveOnly &) = delete;\n> >>>> + MoveOnly &operator=(const MoveOnly &) = delete;\n> >>>> + MoveOnly(MoveOnly &&) = default;\n> >>>> + MoveOnly &operator=(MoveOnly &&) = default;\n> >>>> + };\n> >>>> +\n> >>>> + MoveOnly methodWithReturnMoveOnly()\n> >>>> + {\n> >>>> + return {};\n> >>>> + }\n> >>>> +\n> >>>> private:\n> >>>> Status status_;\n> >>>> int value_;\n> >>>> @@ -186,6 +199,10 @@ protected:\n> >>>> return TestFail;\n> >>>> }\n> >>>>\n> >>>> + /* Test invoking a method that returns type with no copy ctor/assignment. */\n> >>>> + object_.invokeMethod(&InvokedObject::methodWithReturnMoveOnly,\n> >>>> + ConnectionTypeBlocking);\n> >>>> +\n> >>>> return TestPass;\n> >>>> }\n> >>>>\n> >>>> --\n> >>>> 2.50.0\n> >>\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 B0794BE175\n\tfor ;\n\tFri, 18 Jul 2025 17:57:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA7E868FB1;\n\tFri, 18 Jul 2025 19:57: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 2311F6150F\n\tfor ;\n\tFri, 18 Jul 2025 19:57:36 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 131BB1FB2;\n\tFri, 18 Jul 2025 19:57:01 +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=\"FnqcYDx/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752861421;\n\tbh=NY38W1CygKNwswZBdnZVJWzOKMRIeEfO/N4HcXxRJ+s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=FnqcYDx/szHGWSvzn3nrYQVVEOjr3TFg/EApFK4zPoJw0bxNww/OpsX+zxbY0L7LJ\n\tJ9ZyB/ei/UYMyxHa7mZkRQNLQJoXUrfVt9PgahbaE5N3I8oqA6u1kVnXNvuSaRBUyI\n\ti5dfntaQZFNT/jCVltP5p9JMHeEIjf/DlGNXKatA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250619093218.1335931-1-barnabas.pocze@ideasonboard.com>\n\t<175284688979.560048.18345611808889741624@ping.linuxembedded.co.uk>\n\t\n\t<175284767383.560048.6382746607051981355@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v2] libcamera: base: bound_method: Move return value","From":"Kieran Bingham ","Cc":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 18 Jul 2025 18:57:32 +0100","Message-ID":"<175286145240.560048.15679481925900621680@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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\" "}}]