[v1] libcamera: ipa_manager: Store `IPAModule`s in `std::unique_ptr`
diff mbox series

Message ID 20250303193335.785606-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v1] libcamera: ipa_manager: Store `IPAModule`s in `std::unique_ptr`
Related show

Commit Message

Barnabás Pőcze March 3, 2025, 7:33 p.m. UTC 
Express the ownership more clearly by using a smart pointer type.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/ipa_manager.h |  2 +-
 src/libcamera/ipa_manager.cpp            | 18 ++++++------------
 2 files changed, 7 insertions(+), 13 deletions(-)

Comments

Laurent Pinchart March 3, 2025, 9:01 p.m. UTC | #1 
Hi Barnabás,

Thank you for the patch.

On Mon, Mar 03, 2025 at 08:33:35PM +0100, Barnabás Pőcze wrote:
> Express the ownership more clearly by using a smart pointer type.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_manager.h |  2 +-
>  src/libcamera/ipa_manager.cpp            | 18 ++++++------------
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 16dede0c5..e82c25a64 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -67,7 +67,7 @@ private:

#include <memory>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	bool isSignatureValid(IPAModule *ipa) const;
>  
> -	std::vector<IPAModule *> modules_;
> +	std::vector<std::unique_ptr<IPAModule>> modules_;
>  
>  #if HAVE_IPA_PUBKEY
>  	static const uint8_t publicKeyData_[];
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index cfc24d389..830750dcc 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -149,11 +149,7 @@ IPAManager::IPAManager()
>  			<< "No IPA found in '" IPA_MODULE_DIR "'";
>  }
>  
> -IPAManager::~IPAManager()
> -{
> -	for (IPAModule *module : modules_)
> -		delete module;
> -}
> +IPAManager::~IPAManager() = default;
>  
>  /**
>   * \brief Identify shared library objects within a directory
> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>  
>  	unsigned int count = 0;
>  	for (const std::string &file : files) {
> -		IPAModule *ipaModule = new IPAModule(file);
> -		if (!ipaModule->isValid()) {
> -			delete ipaModule;
> +		auto ipaModule = std::make_unique<IPAModule>(file);
> +		if (!ipaModule->isValid())
>  			continue;
> -		}
>  
>  		LOG(IPAManager, Debug) << "Loaded IPA module '" << file << "'";
>  
> -		modules_.push_back(ipaModule);
> +		modules_.push_back(std::move(ipaModule));
>  		count++;
>  	}
>  
> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>  IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>  			      uint32_t maxVersion)
>  {
> -	for (IPAModule *module : modules_) {
> +	for (const auto &module : modules_) {
>  		if (module->match(pipe, minVersion, maxVersion))
> -			return module;
> +			return module.get();
>  	}
>  
>  	return nullptr;
Kieran Bingham March 4, 2025, 10:03 a.m. UTC | #2 
Quoting Barnabás Pőcze (2025-03-03 19:33:35)
> Express the ownership more clearly by using a smart pointer type.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/ipa_manager.h |  2 +-
>  src/libcamera/ipa_manager.cpp            | 18 ++++++------------
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
> index 16dede0c5..e82c25a64 100644
> --- a/include/libcamera/internal/ipa_manager.h
> +++ b/include/libcamera/internal/ipa_manager.h
> @@ -67,7 +67,7 @@ private:
>  
>         bool isSignatureValid(IPAModule *ipa) const;
>  
> -       std::vector<IPAModule *> modules_;
> +       std::vector<std::unique_ptr<IPAModule>> modules_;
>  
>  #if HAVE_IPA_PUBKEY
>         static const uint8_t publicKeyData_[];
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index cfc24d389..830750dcc 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -149,11 +149,7 @@ IPAManager::IPAManager()
>                         << "No IPA found in '" IPA_MODULE_DIR "'";
>  }
>  
> -IPAManager::~IPAManager()
> -{
> -       for (IPAModule *module : modules_)
> -               delete module;
> -}
> +IPAManager::~IPAManager() = default;

Do we have to assign this to default? I thought that was what the
default would be without a destructor ;-)

Or do we normally just put that in the class header?

Anyway, that's an area you likely know better than me so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>  /**
>   * \brief Identify shared library objects within a directory
> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>  
>         unsigned int count = 0;
>         for (const std::string &file : files) {
> -               IPAModule *ipaModule = new IPAModule(file);
> -               if (!ipaModule->isValid()) {
> -                       delete ipaModule;
> +               auto ipaModule = std::make_unique<IPAModule>(file);
> +               if (!ipaModule->isValid())
>                         continue;
> -               }
>  
>                 LOG(IPAManager, Debug) << "Loaded IPA module '" << file << "'";
>  
> -               modules_.push_back(ipaModule);
> +               modules_.push_back(std::move(ipaModule));
>                 count++;
>         }
>  
> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>  IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>                               uint32_t maxVersion)
>  {
> -       for (IPAModule *module : modules_) {
> +       for (const auto &module : modules_) {
>                 if (module->match(pipe, minVersion, maxVersion))
> -                       return module;
> +                       return module.get();
>         }
>  
>         return nullptr;
> -- 
> 2.48.1
>
Barnabás Pőcze March 4, 2025, 1:20 p.m. UTC | #3 
Hi


2025. 03. 04. 11:03 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-03-03 19:33:35)
>> Express the ownership more clearly by using a smart pointer type.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/ipa_manager.h |  2 +-
>>   src/libcamera/ipa_manager.cpp            | 18 ++++++------------
>>   2 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
>> index 16dede0c5..e82c25a64 100644
>> --- a/include/libcamera/internal/ipa_manager.h
>> +++ b/include/libcamera/internal/ipa_manager.h
>> @@ -67,7 +67,7 @@ private:
>>   
>>          bool isSignatureValid(IPAModule *ipa) const;
>>   
>> -       std::vector<IPAModule *> modules_;
>> +       std::vector<std::unique_ptr<IPAModule>> modules_;
>>   
>>   #if HAVE_IPA_PUBKEY
>>          static const uint8_t publicKeyData_[];
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index cfc24d389..830750dcc 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -149,11 +149,7 @@ IPAManager::IPAManager()
>>                          << "No IPA found in '" IPA_MODULE_DIR "'";
>>   }
>>   
>> -IPAManager::~IPAManager()
>> -{
>> -       for (IPAModule *module : modules_)
>> -               delete module;
>> -}
>> +IPAManager::~IPAManager() = default;
> 
> Do we have to assign this to default? I thought that was what the
> default would be without a destructor ;-)

That could be done as well, but I opted to do this because the definition was
already here and so I did not have to remove the declaration in the header.


Regards,
Barnabás Pőcze


> 
> Or do we normally just put that in the class header?
> 
> Anyway, that's an area you likely know better than me so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>>   
>>   /**
>>    * \brief Identify shared library objects within a directory
>> @@ -226,15 +222,13 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>>   
>>          unsigned int count = 0;
>>          for (const std::string &file : files) {
>> -               IPAModule *ipaModule = new IPAModule(file);
>> -               if (!ipaModule->isValid()) {
>> -                       delete ipaModule;
>> +               auto ipaModule = std::make_unique<IPAModule>(file);
>> +               if (!ipaModule->isValid())
>>                          continue;
>> -               }
>>   
>>                  LOG(IPAManager, Debug) << "Loaded IPA module '" << file << "'";
>>   
>> -               modules_.push_back(ipaModule);
>> +               modules_.push_back(std::move(ipaModule));
>>                  count++;
>>          }
>>   
>> @@ -250,9 +244,9 @@ unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
>>   IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
>>                                uint32_t maxVersion)
>>   {
>> -       for (IPAModule *module : modules_) {
>> +       for (const auto &module : modules_) {
>>                  if (module->match(pipe, minVersion, maxVersion))
>> -                       return module;
>> +                       return module.get();
>>          }
>>   
>>          return nullptr;
>> -- 
>> 2.48.1
>>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/ipa_manager.h b/include/libcamera/internal/ipa_manager.h
index 16dede0c5..e82c25a64 100644
--- a/include/libcamera/internal/ipa_manager.h
+++ b/include/libcamera/internal/ipa_manager.h
@@ -67,7 +67,7 @@  private:
 
 	bool isSignatureValid(IPAModule *ipa) const;
 
-	std::vector<IPAModule *> modules_;
+	std::vector<std::unique_ptr<IPAModule>> modules_;
 
 #if HAVE_IPA_PUBKEY
 	static const uint8_t publicKeyData_[];
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index cfc24d389..830750dcc 100644
--- a/src/libcamera/ipa_manager.cpp
+++ b/src/libcamera/ipa_manager.cpp
@@ -149,11 +149,7 @@  IPAManager::IPAManager()
 			<< "No IPA found in '" IPA_MODULE_DIR "'";
 }
 
-IPAManager::~IPAManager()
-{
-	for (IPAModule *module : modules_)
-		delete module;
-}
+IPAManager::~IPAManager() = default;
 
 /**
  * \brief Identify shared library objects within a directory
@@ -226,15 +222,13 @@  unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
 
 	unsigned int count = 0;
 	for (const std::string &file : files) {
-		IPAModule *ipaModule = new IPAModule(file);
-		if (!ipaModule->isValid()) {
-			delete ipaModule;
+		auto ipaModule = std::make_unique<IPAModule>(file);
+		if (!ipaModule->isValid())
 			continue;
-		}
 
 		LOG(IPAManager, Debug) << "Loaded IPA module '" << file << "'";
 
-		modules_.push_back(ipaModule);
+		modules_.push_back(std::move(ipaModule));
 		count++;
 	}
 
@@ -250,9 +244,9 @@  unsigned int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
 IPAModule *IPAManager::module(PipelineHandler *pipe, uint32_t minVersion,
 			      uint32_t maxVersion)
 {
-	for (IPAModule *module : modules_) {
+	for (const auto &module : modules_) {
 		if (module->match(pipe, minVersion, maxVersion))
-			return module;
+			return module.get();
 	}
 
 	return nullptr;