[libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose Extensible private data to other classes

[Laurent Pinchart]

Hi Kieran,

On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote:
> On 05/07/2021 00:28, Laurent Pinchart wrote:
> > Despite sharing the same name, the private data class created by the
> > Extensible design pattern and the C++ private access specifier have
> > different goals. The latter specifies class members private to the
> > class, while the former stores data not visible to the application.
> > 
> > There are use cases for accessing the private data class from other
> > classes inside libcamera. Make this possible by exposing public _d()
> > functions in the class deriving from Extensible. This won't allow access
> > to the private data by applications as the definition of the Private
> > class isn't visible outside of libcamera.
> 
> It almost makes me think the name for the D ptr type should be
> 'Internal', or 'Protected' rather than 'Private', as it's not private
> (it can be accessed) - it's just simply not exposed ...

I knew there would be a naming discussion :-)

The pointer is private to the library, but not private to the class (at
least not in all cases, a particular class doesn't *have* to expose its
Private class if there's no need too). And it's internal to the library
too, but not internal to the class.

> But I'm not opposed to keeping the names we have.
> Ultimately it's the same concept [private: means private to the class,
> Private means private to libcamera as a whole]

I should read through before answering :-)

> > The _d() functions need to be defined as template functions to delay
> > their evaluation, as the static_cast() operator in the Extensible::_d()
> > functions needs the Private class to be fully defined.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/base/class.h | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h
> > index a07dac057331..8212c3d4a5ae 100644
> > --- a/include/libcamera/base/class.h
> > +++ b/include/libcamera/base/class.h
> > @@ -33,14 +33,24 @@ namespace libcamera {
> >  #define LIBCAMERA_DECLARE_PRIVATE()					\
> >  public:									\
> >  	class Private;							\
> > -	friend class Private;
> > +	friend class Private;						\
> > +	template <bool B = true>					\
> 
> Is this some template magic (hack?), to make it a template without
> actually needing to be a template or have any template parameters?
> 
> If so it would be nice to state that in a comment so readers don't
> wonder or query it.

It's explained in the last paragraph of the commit message, would you
like a comment here too ?

> > +	const Private *_d() const					\
> > +	{								\
> > +		return Extensible::_d<Private>();			\
> > +	}								\
> > +	template <bool B = true>					\
> > +	Private *_d()							\
> > +	{								\
> > +		return Extensible::_d<Private>();			\
> > +	}
> >  
> >  #define LIBCAMERA_DECLARE_PUBLIC(klass)					\
> >  	friend class klass;						\
> >  	using Public = klass;
> >  
> >  #define LIBCAMERA_D_PTR()						\
> > -	_d<Private>();
> > +	_d();
> >  
> >  #define LIBCAMERA_O_PTR()						\
> >  	_o<Public>();
> 
> Presumably there wouldn't ever be the need for the equivalent _o() as
> that will only every be used from internally in the Private class.

Correct.

-- 
Regards,

Laurent Pinchart