[libcamera-devel] [PATCH v3 01/14] libcamera: base: extensible: Pass private pointer as unique_ptr<>

[Jacopo Mondi]

Hi Laurent

On Thu, Aug 12, 2021 at 03:39:04PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Aug 12, 2021 at 09:47:25AM +0200, Jacopo Mondi wrote:
> > On Thu, Aug 12, 2021 at 02:26:12AM +0300, Laurent Pinchart wrote:
> > > The Extensible constructor takes a pointer to a Private instance, whose
> > > lifetime it then manages. Make this explicit in the API by passing the
> > > pointer as a std::unique_ptr<Private>.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  include/libcamera/base/class.h    |  2 +-
> > >  src/android/camera_buffer.h       |  2 +-
> > >  src/android/camera_hal_config.cpp |  2 +-
> > >  src/libcamera/base/class.cpp      | 11 +++++++++--
> > >  src/libcamera/camera.cpp          |  2 +-
> > >  src/libcamera/camera_manager.cpp  |  2 +-
> > >  src/libcamera/framebuffer.cpp     |  3 ++-
> > >  7 files changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h
> > > index a5946836d2b9..9a82d61472c1 100644
> > > --- a/include/libcamera/base/class.h
> > > +++ b/include/libcamera/base/class.h
> > > @@ -87,7 +87,7 @@ public:
> > >  		Extensible *const o_;
> > >  	};
> > >
> > > -	Extensible(Private *d);
> > > +	Extensible(std::unique_ptr<Private> d);
> > >
> > >  protected:
> > >  	template<typename T>
> > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > > index 21373fa25bf8..c4e3a9e7b912 100644
> > > --- a/src/android/camera_buffer.h
> > > +++ b/src/android/camera_buffer.h
> > > @@ -32,7 +32,7 @@ public:
> > >
> > >  #define PUBLIC_CAMERA_BUFFER_IMPLEMENTATION				\
> > >  CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)	\
> > > -	: Extensible(new Private(this, camera3Buffer, flags))		\
> > > +	: Extensible(std::make_unique<Private>(this, camera3Buffer, flags)) \
> > >  {									\
> > >  }									\
> > >  CameraBuffer::~CameraBuffer()						\
> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > index 14549083a97a..aa90dac78075 100644
> > > --- a/src/android/camera_hal_config.cpp
> > > +++ b/src/android/camera_hal_config.cpp
> > > @@ -341,7 +341,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,
> > >  }
> > >
> > >  CameraHalConfig::CameraHalConfig()
> > > -	: Extensible(new Private()), exists_(false), valid_(false)
> > > +	: Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > >  {
> > >  	parseConfigurationFile();
> > >  }
> > > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp
> > > index d24043c20e55..9c2d9f218bd4 100644
> > > --- a/src/libcamera/base/class.cpp
> > > +++ b/src/libcamera/base/class.cpp
> > > @@ -147,9 +147,12 @@ namespace libcamera {
> > >  /**
> > >   * \brief Construct an instance of an Extensible class
> > >   * \param[in] d Pointer to the private data instance
> > > + *
> > > + * The private data lifetime is managed by the Extensible class, which destroys
> > > + * it when the Extensible instance is destroyed.
> > >   */
> > > -Extensible::Extensible(Extensible::Private *d)
> > > -	: d_(d)
> > > +Extensible::Extensible(std::unique_ptr<Extensible::Private> d)
> > > +	: d_(std::move(d))
> > >  {
> > >  	*const_cast<Extensible **>(&d_->o_) = this;
> > >  }
> > > @@ -163,6 +166,10 @@ Extensible::Extensible(Extensible::Private *d)
> > >   * overriden _d() functions that return the correct pointer type to the
> > >   * corresponding derived Private class.
> > >   *
> > > + * The lifetime of the private data is tied to the Extensible class. The caller
> > > + * shall not retain any reference to the returned pointer for longer than it
> >
> > is "returned pointer" intentional ?
>
> Yes it is, it's the pointer returned by this function. I'll write "to
> the pointer returned by this function".
>

Nah, don't worry, the diff fooled me, I thought this was the
constructor documentation.