[libcamera-devel,3/8] ipa: camera_sensor_helper: Return unique_ptr from createInstance
diff mbox series

Message ID 20221003212128.32429-4-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: Use class templates for auto-registration
Related show

Commit Message

Laurent Pinchart Oct. 3, 2022, 9:21 p.m. UTC 
Avoid naked pointer with memory allocation by returning a unique_ptr
from CameraSensorHelperFactory::createInstance(), in order to increase
memory allocation safety.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/libipa/camera_sensor_helper.cpp | 7 +++----
 src/ipa/libipa/camera_sensor_helper.h   | 6 +++---
 2 files changed, 6 insertions(+), 7 deletions(-)

Comments

Xavier Roumegue Oct. 5, 2022, 10:59 a.m. UTC | #1 
Hi Laurent,

Thanks for the patch.

On 10/3/22 23:21, Laurent Pinchart via libcamera-devel wrote:
> Avoid naked pointer with memory allocation by returning a unique_ptr
> from CameraSensorHelperFactory::createInstance(), in order to increase
> memory allocation safety.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/ipa/libipa/camera_sensor_helper.cpp | 7 +++----
>   src/ipa/libipa/camera_sensor_helper.h   | 6 +++---
>   2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index fde9bf5b8892..3a7d701d8616 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -261,8 +261,7 @@ std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std:
>   		if (name != factory->name_)
>   			continue;
>   
> -		CameraSensorHelper *helper = factory->createInstance();
> -		return std::unique_ptr<CameraSensorHelper>(helper);
> +		return factory->createInstance();
>   	}
>   
>   	return nullptr;
> @@ -307,8 +306,8 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
>    * macro. It creates a camera sensor helper instance associated with the camera
>    * sensor model.
>    *
> - * \return A pointer to a newly constructed instance of the CameraSensorHelper
> - * subclass corresponding to the factory
> + * \return A unique pointer to a newly constructed instance of the
> + * CameraSensorHelper subclass corresponding to the factory
>    */
>   
>   /**
> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> index 410156efb2ea..21ee43cc9f9f 100644
> --- a/src/ipa/libipa/camera_sensor_helper.h
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -73,7 +73,7 @@ private:
>   
>   	static void registerType(CameraSensorHelperFactory *factory);
>   
> -	virtual CameraSensorHelper *createInstance() const = 0;
> +	virtual std::unique_ptr<CameraSensorHelper> createInstance() const = 0;
>   
>   	std::string name_;
>   };
> @@ -85,9 +85,9 @@ public: 							\
>   	helper##Factory() : CameraSensorHelperFactory(name) {}	\
>   								\
>   private:							\
> -	CameraSensorHelper *createInstance() const		\
> +	std::unique_ptr<CameraSensorHelper> createInstance() const \
>   	{							\
> -		return new helper();				\
> +		return std::make_unique<helper>();		\
>   	}							\
>   };								\
>   static helper##Factory global_##helper##Factory;


Reviewed-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
Jacopo Mondi Oct. 7, 2022, 1:40 p.m. UTC | #2 
Hi Laurent

On Tue, Oct 04, 2022 at 12:21:23AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Avoid naked pointer with memory allocation by returning a unique_ptr
> from CameraSensorHelperFactory::createInstance(), in order to increase
> memory allocation safety.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks fine
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  src/ipa/libipa/camera_sensor_helper.cpp | 7 +++----
>  src/ipa/libipa/camera_sensor_helper.h   | 6 +++---
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index fde9bf5b8892..3a7d701d8616 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -261,8 +261,7 @@ std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std:
>  		if (name != factory->name_)
>  			continue;
>
> -		CameraSensorHelper *helper = factory->createInstance();
> -		return std::unique_ptr<CameraSensorHelper>(helper);
> +		return factory->createInstance();
>  	}
>
>  	return nullptr;
> @@ -307,8 +306,8 @@ std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
>   * macro. It creates a camera sensor helper instance associated with the camera
>   * sensor model.
>   *
> - * \return A pointer to a newly constructed instance of the CameraSensorHelper
> - * subclass corresponding to the factory
> + * \return A unique pointer to a newly constructed instance of the
> + * CameraSensorHelper subclass corresponding to the factory
>   */
>
>  /**
> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> index 410156efb2ea..21ee43cc9f9f 100644
> --- a/src/ipa/libipa/camera_sensor_helper.h
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -73,7 +73,7 @@ private:
>
>  	static void registerType(CameraSensorHelperFactory *factory);
>
> -	virtual CameraSensorHelper *createInstance() const = 0;
> +	virtual std::unique_ptr<CameraSensorHelper> createInstance() const = 0;
>
>  	std::string name_;
>  };
> @@ -85,9 +85,9 @@ public: 							\
>  	helper##Factory() : CameraSensorHelperFactory(name) {}	\
>  								\
>  private:							\
> -	CameraSensorHelper *createInstance() const		\
> +	std::unique_ptr<CameraSensorHelper> createInstance() const \
>  	{							\
> -		return new helper();				\
> +		return std::make_unique<helper>();		\
>  	}							\
>  };								\
>  static helper##Factory global_##helper##Factory;
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index fde9bf5b8892..3a7d701d8616 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -261,8 +261,7 @@  std::unique_ptr<CameraSensorHelper> CameraSensorHelperFactory::create(const std:
 		if (name != factory->name_)
 			continue;
 
-		CameraSensorHelper *helper = factory->createInstance();
-		return std::unique_ptr<CameraSensorHelper>(helper);
+		return factory->createInstance();
 	}
 
 	return nullptr;
@@ -307,8 +306,8 @@  std::vector<CameraSensorHelperFactory *> &CameraSensorHelperFactory::factories()
  * macro. It creates a camera sensor helper instance associated with the camera
  * sensor model.
  *
- * \return A pointer to a newly constructed instance of the CameraSensorHelper
- * subclass corresponding to the factory
+ * \return A unique pointer to a newly constructed instance of the
+ * CameraSensorHelper subclass corresponding to the factory
  */
 
 /**
diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
index 410156efb2ea..21ee43cc9f9f 100644
--- a/src/ipa/libipa/camera_sensor_helper.h
+++ b/src/ipa/libipa/camera_sensor_helper.h
@@ -73,7 +73,7 @@  private:
 
 	static void registerType(CameraSensorHelperFactory *factory);
 
-	virtual CameraSensorHelper *createInstance() const = 0;
+	virtual std::unique_ptr<CameraSensorHelper> createInstance() const = 0;
 
 	std::string name_;
 };
@@ -85,9 +85,9 @@  public: 							\
 	helper##Factory() : CameraSensorHelperFactory(name) {}	\
 								\
 private:							\
-	CameraSensorHelper *createInstance() const		\
+	std::unique_ptr<CameraSensorHelper> createInstance() const \
 	{							\
-		return new helper();				\
+		return std::make_unique<helper>();		\
 	}							\
 };								\
 static helper##Factory global_##helper##Factory;