[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 01:26:58PM +0100, Kieran Bingham wrote:
> On 07/07/2021 12:46, Laurent Pinchart wrote:
> > 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 ?
> 
> The commit message explains that we need to use templates to delay
> evaluation.
> 
> But <bool B = true> looks like a magic hack, and otherwise out of place.
> 
> Reading this file without the commit message, it's really unclear why
> there is a bool B = true, or even what it's supposed to do. (or more
> importantly, that it's supposed to be simply ignored)

I'll expand the explanation in the commit message. Would you prefer also
having a comment here ? I would keep it short then.

> >>> +	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