[libcamera-devel,v1.1,2/4] libcamera: camera_manager: Use std::unique_ptr to store event dispatcher
diff mbox series

Message ID 20190118003201.3110-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Untitled series #91
Related show

Commit Message

Laurent Pinchart Jan. 18, 2019, 12:32 a.m. UTC 
The CameraManager takes ownership of the dispatcher passed to the
setEventDispatcher() function. Enforces this by using std::unique_ptr<>.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Use make_unique<> in CameraManager::setEventDispatcher()
---
 include/libcamera/camera_manager.h |  4 ++--
 src/libcamera/camera_manager.cpp   | 12 +++++++-----
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Jacopo Mondi Jan. 18, 2019, 3:15 p.m. UTC | #1 
Hi Laurent,
   one quick question

On Fri, Jan 18, 2019 at 02:32:01AM +0200, Laurent Pinchart wrote:
> The CameraManager takes ownership of the dispatcher passed to the
> setEventDispatcher() function. Enforces this by using std::unique_ptr<>.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Use make_unique<> in CameraManager::setEventDispatcher()
> ---
>  include/libcamera/camera_manager.h |  4 ++--
>  src/libcamera/camera_manager.cpp   | 12 +++++++-----
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 6cfcba3c3933..b82a8ce95b9f 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -29,7 +29,7 @@ public:
>
>  	static CameraManager *instance();
>
> -	void setEventDispatcher(EventDispatcher *dispatcher);
> +	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  	EventDispatcher *eventDispatcher();
>
>  private:
> @@ -41,7 +41,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
>
> -	EventDispatcher *dispatcher_;
> +	std::unique_ptr<EventDispatcher> dispatcher_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 9f554be57e33..1430bb0d75a5 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -12,6 +12,7 @@
>  #include "event_dispatcher_poll.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
> +#include "utils.h"
>
>  /**
>   * \file camera_manager.h
> @@ -58,7 +59,6 @@ CameraManager::CameraManager()
>
>  CameraManager::~CameraManager()
>  {
> -	delete dispatcher_;
>  }
>
>  /**
> @@ -209,14 +209,14 @@ CameraManager *CameraManager::instance()
>   * The CameraManager takes ownership of the event dispatcher and will delete it
>   * when the application terminates.
>   */
> -void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> +void CameraManager::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
>  {
>  	if (dispatcher_) {
>  		LOG(Warning) << "Event dispatcher is already set";
>  		return;
>  	}
>
> -	dispatcher_ = dispatcher;
> +	dispatcher_ = std::move(dispatcher);
>  }
>
>  /**
> @@ -226,14 +226,16 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
>   * If no dispatcher has been set, a default poll-based implementation is created
>   * and returned, and no custom event dispatcher may be installed anymore.
>   *
> + * The returned event dispatcher is valid until the camera manager is destroyed.
> + *
>   * \return Pointer to the event dispatcher
>   */
>  EventDispatcher *CameraManager::eventDispatcher()
>  {
>  	if (!dispatcher_)

Does this access the resources owned by the unique_ptr<> ?

Thanks
   j

> -		dispatcher_ = new EventDispatcherPoll();
> +		dispatcher_ = utils::make_unique<EventDispatcherPoll>();
>
> -	return dispatcher_;
> +	return dispatcher_.get();
>  }
>
>  } /* namespace libcamera */
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 18, 2019, 3:31 p.m. UTC | #2 
Hi Jacopo,

On Fri, Jan 18, 2019 at 04:15:03PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 18, 2019 at 02:32:01AM +0200, Laurent Pinchart wrote:
> > The CameraManager takes ownership of the dispatcher passed to the
> > setEventDispatcher() function. Enforces this by using std::unique_ptr<>.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Use make_unique<> in CameraManager::setEventDispatcher()
> > ---
> >  include/libcamera/camera_manager.h |  4 ++--
> >  src/libcamera/camera_manager.cpp   | 12 +++++++-----
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > index 6cfcba3c3933..b82a8ce95b9f 100644
> > --- a/include/libcamera/camera_manager.h
> > +++ b/include/libcamera/camera_manager.h
> > @@ -29,7 +29,7 @@ public:
> >
> >  	static CameraManager *instance();
> >
> > -	void setEventDispatcher(EventDispatcher *dispatcher);
> > +	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
> >  	EventDispatcher *eventDispatcher();
> >
> >  private:
> > @@ -41,7 +41,7 @@ private:
> >  	std::unique_ptr<DeviceEnumerator> enumerator_;
> >  	std::vector<PipelineHandler *> pipes_;
> >
> > -	EventDispatcher *dispatcher_;
> > +	std::unique_ptr<EventDispatcher> dispatcher_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index 9f554be57e33..1430bb0d75a5 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -12,6 +12,7 @@
> >  #include "event_dispatcher_poll.h"
> >  #include "log.h"
> >  #include "pipeline_handler.h"
> > +#include "utils.h"
> >
> >  /**
> >   * \file camera_manager.h
> > @@ -58,7 +59,6 @@ CameraManager::CameraManager()
> >
> >  CameraManager::~CameraManager()
> >  {
> > -	delete dispatcher_;
> >  }
> >
> >  /**
> > @@ -209,14 +209,14 @@ CameraManager *CameraManager::instance()
> >   * The CameraManager takes ownership of the event dispatcher and will delete it
> >   * when the application terminates.
> >   */
> > -void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> > +void CameraManager::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
> >  {
> >  	if (dispatcher_) {
> >  		LOG(Warning) << "Event dispatcher is already set";
> >  		return;
> >  	}
> >
> > -	dispatcher_ = dispatcher;
> > +	dispatcher_ = std::move(dispatcher);
> >  }
> >
> >  /**
> > @@ -226,14 +226,16 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> >   * If no dispatcher has been set, a default poll-based implementation is created
> >   * and returned, and no custom event dispatcher may be installed anymore.
> >   *
> > + * The returned event dispatcher is valid until the camera manager is destroyed.
> > + *
> >   * \return Pointer to the event dispatcher
> >   */
> >  EventDispatcher *CameraManager::eventDispatcher()
> >  {
> >  	if (!dispatcher_)
> 
> Does this access the resources owned by the unique_ptr<> ?

If you mean does this check whether the pointer owned by the
std::unique_ptr<> dispatch_ is null, then yes, it does, through operator
bool:

https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool

If you meant something else, then please rephrase the question :-)

> > -		dispatcher_ = new EventDispatcherPoll();
> > +		dispatcher_ = utils::make_unique<EventDispatcherPoll>();
> >
> > -	return dispatcher_;
> > +	return dispatcher_.get();
> >  }
> >
> >  } /* namespace libcamera */
Jacopo Mondi Jan. 18, 2019, 3:42 p.m. UTC | #3 
Hi Laurent,

On Fri, Jan 18, 2019 at 05:31:17PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jan 18, 2019 at 04:15:03PM +0100, Jacopo Mondi wrote:
> > On Fri, Jan 18, 2019 at 02:32:01AM +0200, Laurent Pinchart wrote:
> > > The CameraManager takes ownership of the dispatcher passed to the
> > > setEventDispatcher() function. Enforces this by using std::unique_ptr<>.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Use make_unique<> in CameraManager::setEventDispatcher()
> > > ---
> > >  include/libcamera/camera_manager.h |  4 ++--
> > >  src/libcamera/camera_manager.cpp   | 12 +++++++-----
> > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> > > index 6cfcba3c3933..b82a8ce95b9f 100644
> > > --- a/include/libcamera/camera_manager.h
> > > +++ b/include/libcamera/camera_manager.h
> > > @@ -29,7 +29,7 @@ public:
> > >
> > >  	static CameraManager *instance();
> > >
> > > -	void setEventDispatcher(EventDispatcher *dispatcher);
> > > +	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
> > >  	EventDispatcher *eventDispatcher();
> > >
> > >  private:
> > > @@ -41,7 +41,7 @@ private:
> > >  	std::unique_ptr<DeviceEnumerator> enumerator_;
> > >  	std::vector<PipelineHandler *> pipes_;
> > >
> > > -	EventDispatcher *dispatcher_;
> > > +	std::unique_ptr<EventDispatcher> dispatcher_;
> > >  };
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > > index 9f554be57e33..1430bb0d75a5 100644
> > > --- a/src/libcamera/camera_manager.cpp
> > > +++ b/src/libcamera/camera_manager.cpp
> > > @@ -12,6 +12,7 @@
> > >  #include "event_dispatcher_poll.h"
> > >  #include "log.h"
> > >  #include "pipeline_handler.h"
> > > +#include "utils.h"
> > >
> > >  /**
> > >   * \file camera_manager.h
> > > @@ -58,7 +59,6 @@ CameraManager::CameraManager()
> > >
> > >  CameraManager::~CameraManager()
> > >  {
> > > -	delete dispatcher_;
> > >  }
> > >
> > >  /**
> > > @@ -209,14 +209,14 @@ CameraManager *CameraManager::instance()
> > >   * The CameraManager takes ownership of the event dispatcher and will delete it
> > >   * when the application terminates.
> > >   */
> > > -void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> > > +void CameraManager::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
> > >  {
> > >  	if (dispatcher_) {
> > >  		LOG(Warning) << "Event dispatcher is already set";
> > >  		return;
> > >  	}
> > >
> > > -	dispatcher_ = dispatcher;
> > > +	dispatcher_ = std::move(dispatcher);
> > >  }
> > >
> > >  /**
> > > @@ -226,14 +226,16 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> > >   * If no dispatcher has been set, a default poll-based implementation is created
> > >   * and returned, and no custom event dispatcher may be installed anymore.
> > >   *
> > > + * The returned event dispatcher is valid until the camera manager is destroyed.
> > > + *
> > >   * \return Pointer to the event dispatcher
> > >   */
> > >  EventDispatcher *CameraManager::eventDispatcher()
> > >  {
> > >  	if (!dispatcher_)
> >
> > Does this access the resources owned by the unique_ptr<> ?
>
> If you mean does this check whether the pointer owned by the
> std::unique_ptr<> dispatch_ is null, then yes, it does, through operator
> bool:
>
> https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool
>
> If you meant something else, then please rephrase the question :-)

That was my question :)
Thanks
  j

>
> > > -		dispatcher_ = new EventDispatcherPoll();
> > > +		dispatcher_ = utils::make_unique<EventDispatcherPoll>();
> > >
> > > -	return dispatcher_;
> > > +	return dispatcher_.get();
> > >  }
> > >
> > >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
Niklas Söderlund Jan. 18, 2019, 3:50 p.m. UTC | #4 
Hi Laurent,

Thanks for your patch.

On 2019-01-18 02:32:01 +0200, Laurent Pinchart wrote:
> The CameraManager takes ownership of the dispatcher passed to the
> setEventDispatcher() function. Enforces this by using std::unique_ptr<>.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> Changes since v1:
> 
> - Use make_unique<> in CameraManager::setEventDispatcher()
> ---
>  include/libcamera/camera_manager.h |  4 ++--
>  src/libcamera/camera_manager.cpp   | 12 +++++++-----
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
> index 6cfcba3c3933..b82a8ce95b9f 100644
> --- a/include/libcamera/camera_manager.h
> +++ b/include/libcamera/camera_manager.h
> @@ -29,7 +29,7 @@ public:
>  
>  	static CameraManager *instance();
>  
> -	void setEventDispatcher(EventDispatcher *dispatcher);
> +	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
>  	EventDispatcher *eventDispatcher();
>  
>  private:
> @@ -41,7 +41,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::vector<PipelineHandler *> pipes_;
>  
> -	EventDispatcher *dispatcher_;
> +	std::unique_ptr<EventDispatcher> dispatcher_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 9f554be57e33..1430bb0d75a5 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -12,6 +12,7 @@
>  #include "event_dispatcher_poll.h"
>  #include "log.h"
>  #include "pipeline_handler.h"
> +#include "utils.h"
>  
>  /**
>   * \file camera_manager.h
> @@ -58,7 +59,6 @@ CameraManager::CameraManager()
>  
>  CameraManager::~CameraManager()
>  {
> -	delete dispatcher_;
>  }
>  
>  /**
> @@ -209,14 +209,14 @@ CameraManager *CameraManager::instance()
>   * The CameraManager takes ownership of the event dispatcher and will delete it
>   * when the application terminates.
>   */
> -void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
> +void CameraManager::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
>  {
>  	if (dispatcher_) {
>  		LOG(Warning) << "Event dispatcher is already set";
>  		return;
>  	}
>  
> -	dispatcher_ = dispatcher;
> +	dispatcher_ = std::move(dispatcher);
>  }
>  
>  /**
> @@ -226,14 +226,16 @@ void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
>   * If no dispatcher has been set, a default poll-based implementation is created
>   * and returned, and no custom event dispatcher may be installed anymore.
>   *
> + * The returned event dispatcher is valid until the camera manager is destroyed.
> + *
>   * \return Pointer to the event dispatcher
>   */
>  EventDispatcher *CameraManager::eventDispatcher()
>  {
>  	if (!dispatcher_)
> -		dispatcher_ = new EventDispatcherPoll();
> +		dispatcher_ = utils::make_unique<EventDispatcherPoll>();
>  
> -	return dispatcher_;
> +	return dispatcher_.get();
>  }
>  
>  } /* namespace libcamera */
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index 6cfcba3c3933..b82a8ce95b9f 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -29,7 +29,7 @@  public:
 
 	static CameraManager *instance();
 
-	void setEventDispatcher(EventDispatcher *dispatcher);
+	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
 	EventDispatcher *eventDispatcher();
 
 private:
@@ -41,7 +41,7 @@  private:
 	std::unique_ptr<DeviceEnumerator> enumerator_;
 	std::vector<PipelineHandler *> pipes_;
 
-	EventDispatcher *dispatcher_;
+	std::unique_ptr<EventDispatcher> dispatcher_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 9f554be57e33..1430bb0d75a5 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -12,6 +12,7 @@ 
 #include "event_dispatcher_poll.h"
 #include "log.h"
 #include "pipeline_handler.h"
+#include "utils.h"
 
 /**
  * \file camera_manager.h
@@ -58,7 +59,6 @@  CameraManager::CameraManager()
 
 CameraManager::~CameraManager()
 {
-	delete dispatcher_;
 }
 
 /**
@@ -209,14 +209,14 @@  CameraManager *CameraManager::instance()
  * The CameraManager takes ownership of the event dispatcher and will delete it
  * when the application terminates.
  */
-void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
+void CameraManager::setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher)
 {
 	if (dispatcher_) {
 		LOG(Warning) << "Event dispatcher is already set";
 		return;
 	}
 
-	dispatcher_ = dispatcher;
+	dispatcher_ = std::move(dispatcher);
 }
 
 /**
@@ -226,14 +226,16 @@  void CameraManager::setEventDispatcher(EventDispatcher *dispatcher)
  * If no dispatcher has been set, a default poll-based implementation is created
  * and returned, and no custom event dispatcher may be installed anymore.
  *
+ * The returned event dispatcher is valid until the camera manager is destroyed.
+ *
  * \return Pointer to the event dispatcher
  */
 EventDispatcher *CameraManager::eventDispatcher()
 {
 	if (!dispatcher_)
-		dispatcher_ = new EventDispatcherPoll();
+		dispatcher_ = utils::make_unique<EventDispatcherPoll>();
 
-	return dispatcher_;
+	return dispatcher_.get();
 }
 
 } /* namespace libcamera */