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

[Kieran Bingham]

Hi Laurent,

On 07/07/2021 12:46, Laurent Pinchart wrote:
> 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 ?

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)


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