[{"id":33553,"web_url":"https://patchwork.libcamera.org/comment/33553/","msgid":"<20250303210157.GF23684@pendragon.ideasonboard.com>","date":"2025-03-03T21:01:57","subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Mar 03, 2025 at 08:33:35PM +0100, Barnabás Pőcze wrote:\n> Express the ownership more clearly by using a smart pointer type.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/ipa_manager.h | 2 +-\n> src/libcamera/ipa_manager.cpp | 18 ++++++------------\n> 2 files changed, 7 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index 16dede0c5..e82c25a64 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -67,7 +67,7 @@ private:\n\n#include \n\nReviewed-by: Laurent Pinchart \n\n> \n> \tbool isSignatureValid(IPAModule *ipa) const;\n> \n> -\tstd::vector modules_;\n> +\tstd::vector> modules_;\n> \n> #if HAVE_IPA_PUBKEY\n> \tstatic const uint8_t publicKeyData_[];\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index cfc24d389..830750dcc 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -149,11 +149,7 @@ IPAManager::IPAManager()\n> \t\t\t<< \"No IPA found in '\" IPA_MODULE_DIR \"'\";\n> }\n> \n> -IPAManager::~IPAManager()\n> -{\n> -\tfor (IPAModule *module : modules_)\n> -\t\tdelete module;\n> -}\n> +IPAManager::~IPAManager() = default;\n> \n> /**\n> * \\brief Identify shared library objects within a directory\n> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> \n> \tunsigned int count = 0;\n> \tfor (const std::string &file : files) {\n> -\t\tIPAModule *ipaModule = new IPAModule(file);\n> -\t\tif (!ipaModule->isValid()) {\n> -\t\t\tdelete ipaModule;\n> +\t\tauto ipaModule = std::make_unique(file);\n> +\t\tif (!ipaModule->isValid())\n> \t\t\tcontinue;\n> -\t\t}\n> \n> \t\tLOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> \n> -\t\tmodules_.push_back(ipaModule);\n> +\t\tmodules_.push_back(std::move(ipaModule));\n> \t\tcount++;\n> \t}\n> \n> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> \t\t\t uint32_t maxVersion)\n> {\n> -\tfor (IPAModule *module : modules_) {\n> +\tfor (const auto &module : modules_) {\n> \t\tif (module->match(pipe, minVersion, maxVersion))\n> -\t\t\treturn module;\n> +\t\t\treturn module.get();\n> \t}\n> \n> \treturn nullptr;","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 E4C53BD808\n\tfor ;\n\tMon, 3 Mar 2025 21:02:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 367C068822;\n\tMon, 3 Mar 2025 22:02:21 +0100 (CET)","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 437E068772\n\tfor ;\n\tMon, 3 Mar 2025 22:02:19 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69A888DB;\n\tMon, 3 Mar 2025 22:00:47 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ovQVmWUn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741035647;\n\tbh=oXJ3TsSgjTJo0zD7UW7xFy2JBfTNla1PNWycUa3VLhs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ovQVmWUnVVD6KWmJ3D1t1tANhgY6zltuaUVr0hbCqhGEjb8CA9HVUYFqedZkhdveA\n\tgwCWqDML0EuUYbIAtIAO6gVfjzYsyE6HU440jNW4XhdXlxhlwg2T1gS7hXaWL8glr7\n\tjmocY82K3IUc3jtTDusNXHdIHZfrEaN417EZtm/w=","Date":"Mon, 3 Mar 2025 23:01:57 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","Message-ID":"<20250303210157.GF23684@pendragon.ideasonboard.com>","References":"<20250303193335.785606-1-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250303193335.785606-1-barnabas.pocze@ideasonboard.com>","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":33562,"web_url":"https://patchwork.libcamera.org/comment/33562/","msgid":"<174108263678.2914008.6041658864807866544@ping.linuxembedded.co.uk>","date":"2025-03-04T10:03:56","subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","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-03-03 19:33:35)\n> Express the ownership more clearly by using a smart pointer type.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> include/libcamera/internal/ipa_manager.h | 2 +-\n> src/libcamera/ipa_manager.cpp | 18 ++++++------------\n> 2 files changed, 7 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n> index 16dede0c5..e82c25a64 100644\n> --- a/include/libcamera/internal/ipa_manager.h\n> +++ b/include/libcamera/internal/ipa_manager.h\n> @@ -67,7 +67,7 @@ private:\n> \n> bool isSignatureValid(IPAModule *ipa) const;\n> \n> - std::vector modules_;\n> + std::vector> modules_;\n> \n> #if HAVE_IPA_PUBKEY\n> static const uint8_t publicKeyData_[];\n> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n> index cfc24d389..830750dcc 100644\n> --- a/src/libcamera/ipa_manager.cpp\n> +++ b/src/libcamera/ipa_manager.cpp\n> @@ -149,11 +149,7 @@ IPAManager::IPAManager()\n> << \"No IPA found in '\" IPA_MODULE_DIR \"'\";\n> }\n> \n> -IPAManager::~IPAManager()\n> -{\n> - for (IPAModule *module : modules_)\n> - delete module;\n> -}\n> +IPAManager::~IPAManager() = default;\n\nDo we have to assign this to default? I thought that was what the\ndefault would be without a destructor ;-)\n\nOr do we normally just put that in the class header?\n\nAnyway, that's an area you likely know better than me so:\n\nReviewed-by: Kieran Bingham \n\n> \n> /**\n> * \\brief Identify shared library objects within a directory\n> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> \n> unsigned int count = 0;\n> for (const std::string &file : files) {\n> - IPAModule *ipaModule = new IPAModule(file);\n> - if (!ipaModule->isValid()) {\n> - delete ipaModule;\n> + auto ipaModule = std::make_unique(file);\n> + if (!ipaModule->isValid())\n> continue;\n> - }\n> \n> LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n> \n> - modules_.push_back(ipaModule);\n> + modules_.push_back(std::move(ipaModule));\n> count++;\n> }\n> \n> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n> uint32_t maxVersion)\n> {\n> - for (IPAModule *module : modules_) {\n> + for (const auto &module : modules_) {\n> if (module->match(pipe, minVersion, maxVersion))\n> - return module;\n> + return module.get();\n> }\n> \n> return nullptr;\n> -- \n> 2.48.1\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 7F97BC3257\n\tfor ;\n\tTue, 4 Mar 2025 10:04:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94BD268779;\n\tTue, 4 Mar 2025 11:04:01 +0100 (CET)","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 2308F68754\n\tfor ;\n\tTue, 4 Mar 2025 11:04:00 +0100 (CET)","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 E81061083;\n\tTue, 4 Mar 2025 11:02:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UhWGF+kU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741082548;\n\tbh=hhO6DiQS6xFzUDYT1ivXpkk++2Yjog4xi0aXW98kV00=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=UhWGF+kULPUOJApTUT4WXErJTcGflDEFQZ7zelUBK15eUP47mMz60MzK6IKkQ7KJH\n\tbTcB1Hw5xsudTUZogAGlFYtiIr5TTboYuD7CBVq7QN/Es6LX43+FVhQGYY7BdCzppu\n\t1u5CziDJrajgbNjRpoikvPJROP7krBHqm3RxsGf4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250303193335.785606-1-barnabas.pocze@ideasonboard.com>","References":"<20250303193335.785606-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 04 Mar 2025 10:03:56 +0000","Message-ID":"<174108263678.2914008.6041658864807866544@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33572,"web_url":"https://patchwork.libcamera.org/comment/33572/","msgid":"<07961a39-dd08-4a6d-8675-fabf0e3a2ed6@ideasonboard.com>","date":"2025-03-04T13:20:10","subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 04. 11:03 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-03-03 19:33:35)\n>> Express the ownership more clearly by using a smart pointer type.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> include/libcamera/internal/ipa_manager.h | 2 +-\n>> src/libcamera/ipa_manager.cpp | 18 ++++++------------\n>> 2 files changed, 7 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h\n>> index 16dede0c5..e82c25a64 100644\n>> --- a/include/libcamera/internal/ipa_manager.h\n>> +++ b/include/libcamera/internal/ipa_manager.h\n>> @@ -67,7 +67,7 @@ private:\n>> \n>> bool isSignatureValid(IPAModule *ipa) const;\n>> \n>> - std::vector modules_;\n>> + std::vector> modules_;\n>> \n>> #if HAVE_IPA_PUBKEY\n>> static const uint8_t publicKeyData_[];\n>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\n>> index cfc24d389..830750dcc 100644\n>> --- a/src/libcamera/ipa_manager.cpp\n>> +++ b/src/libcamera/ipa_manager.cpp\n>> @@ -149,11 +149,7 @@ IPAManager::IPAManager()\n>> << \"No IPA found in '\" IPA_MODULE_DIR \"'\";\n>> }\n>> \n>> -IPAManager::~IPAManager()\n>> -{\n>> - for (IPAModule *module : modules_)\n>> - delete module;\n>> -}\n>> +IPAManager::~IPAManager() = default;\n> \n> Do we have to assign this to default? I thought that was what the\n> default would be without a destructor ;-)\n\nThat could be done as well, but I opted to do this because the definition was\nalready here and so I did not have to remove the declaration in the header.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Or do we normally just put that in the class header?\n> \n> Anyway, that's an area you likely know better than me so:\n> \n> Reviewed-by: Kieran Bingham \n> \n>> \n>> /**\n>> * \\brief Identify shared library objects within a directory\n>> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n>> \n>> unsigned int count = 0;\n>> for (const std::string &file : files) {\n>> - IPAModule *ipaModule = new IPAModule(file);\n>> - if (!ipaModule->isValid()) {\n>> - delete ipaModule;\n>> + auto ipaModule = std::make_unique(file);\n>> + if (!ipaModule->isValid())\n>> continue;\n>> - }\n>> \n>> LOG(IPAManager, Debug) << \"Loaded IPA module '\" << file << \"'\";\n>> \n>> - modules_.push_back(ipaModule);\n>> + modules_.push_back(std::move(ipaModule));\n>> count++;\n>> }\n>> \n>> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)\n>> IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,\n>> uint32_t maxVersion)\n>> {\n>> - for (IPAModule *module : modules_) {\n>> + for (const auto &module : modules_) {\n>> if (module->match(pipe, minVersion, maxVersion))\n>> - return module;\n>> + return module.get();\n>> }\n>> \n>> return nullptr;\n>> -- \n>> 2.48.1\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 9DC85C32DC\n\tfor ;\n\tTue, 4 Mar 2025 13:20:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45B2C68779;\n\tTue, 4 Mar 2025 14:20:16 +0100 (CET)","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 E940068755\n\tfor ;\n\tTue, 4 Mar 2025 14:20:13 +0100 (CET)","from [192.168.33.3] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBA2F8FA;\n\tTue, 4 Mar 2025 14:18:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dbuBBAjP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741094321;\n\tbh=EYbqHBbWmjSOcXeIDobZWcEca8qOhR5APP+zDiPlZIQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=dbuBBAjPrBFMdisUqgnqTq+64ufUmAPWiMzs4OLBhCukztBsbO35QAhVVSQDTYbOw\n\t8oLSTEEvswobHmsBta+HoFe7IshUrEUZPj+H5jd3b3qatpX1RvbpFJyfT8a7hLSUHu\n\tGIRd0SYjioF7ee3QwMievVou4Zc0UcHtwTUidiIg=","Message-ID":"<07961a39-dd08-4a6d-8675-fabf0e3a2ed6@ideasonboard.com>","Date":"Tue, 4 Mar 2025 14:20:10 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: ipa_manager: Store `IPAModule`s in\n\t`std::unique_ptr`","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250303193335.785606-1-barnabas.pocze@ideasonboard.com>\n\t<174108263678.2914008.6041658864807866544@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174108263678.2914008.6041658864807866544@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\" "}}]