[libcamera-devel] [PATCH 1/4] libcamera: Add a base class to implement the d-pointer design pattern

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 21, 2020 at 11:25:48AM +0200, Jacopo Mondi wrote:
> On Mon, Sep 21, 2020 at 06:10:54AM +0300, Laurent Pinchart wrote:
> > The d-pointer design patterns helps creating public classes that can be
> > extended without breaking their ABI. To facilitate usage of the pattern
> > in libcamera, create a base Extensible class with associated macros.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/extensible.h |  80 +++++++++++++++++++++
> >  include/libcamera/meson.build  |   1 +
> >  src/libcamera/extensible.cpp   | 127 +++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build      |   1 +
> >  4 files changed, 209 insertions(+)
> >  create mode 100644 include/libcamera/extensible.h
> >  create mode 100644 src/libcamera/extensible.cpp
> >
> > diff --git a/include/libcamera/extensible.h b/include/libcamera/extensible.h
> > new file mode 100644
> > index 000000000000..0fa3bcdbeac5
> > --- /dev/null
> > +++ b/include/libcamera/extensible.h
> > @@ -0,0 +1,80 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * extensible.h - Utilities to create extensible public classes with stable ABIs
> > + */
> > +#ifndef __LIBCAMERA_EXTENSIBLE_H__
> > +#define __LIBCAMERA_EXTENSIBLE_H__
> > +
> > +#include <memory>
> > +
> > +namespace libcamera {
> > +
> > +#ifndef __DOXYGEN__
> > +#define LIBCAMERA_DECLARE_PRIVATE(klass)				\
> > +public:									\
> > +	class Private;							\
> > +	friend class Private;
> > +
> > +#define LIBCAMERA_DECLARE_PUBLIC(klass)					\
> > +private:								\
> > +	friend class klass;						\
> 
> Is this necessary ? The Exensible 'klass' can access the Private one,
> doesn't it ?

The idea is that the private class is fully opaque to applications, but
may need to be accessed by other internal libcamera classes. When that
happens, it would likely need to declare different members with
different visibility, only exposing as public the API that internal
classes may use, while keeping the rest private. The private data,
however, will always need to be accessed by the public class. This is
what this friend statement is for

class MyPublicClass
{
	LIBCAMERA_DECLARE_PRIVATE(MyPublicClass)

public:
	...
};

...

class MyPublicClass::Private
{
	LIBCAMERA_DECLARE_PUBLIC(MyPublicClass)
public:
	int foo() const { return foo_; }

private:
	int foo_;
};

The usage pattern would be that MyPublicClass::Private::foo() is
accessible by other libcamera internal classes, while setting
MyPublicClass::Private::foo_ would only be allowed for MyPublicClass.

> class Extensible
> {
> private:
>         class Private
>         {
> 
>         };
> };
> 
> > +	const klass *_o() const						\
> > +	{								\
> > +		return static_cast<const klass *>(o_);			\
> > +	}								\
> > +	klass *_o()							\
> > +	{								\
> > +		return static_cast<klass *>(o_);			\
> > +	}
> > +
> > +#define LIBCAMERA_D_PTR(klass)						\
> > +	auto * const d = _d<klass::Private>();
> 
> Can't we remove the template here? If instead of klass::Private you
> just use Private would not that be resolved to 'klass'::Private (as
> this is meant to be used inside the 'klass' scope) ? Or does this
> break with classes that inherits from an Extensible derived class ?

I think that's fine. We need to keep the template, but we can write
_d<Private> instead of _d<klass::Private>. This would allow dropping the
klass argument to the macro.

> > +
> > +#define LIBCAMERA_O_PTR(klass)						\
> > +	auto * const o = _o();
> 
> Is the 'klass' parameter used ? This is meant to be called from
> withing the Private class right ? the LIBCAMERA_DECLARE_PUBLIC()
> macro already define the signature of the _o() method, so you don't
> need klass I think. Is this to force the user to be aware of the type
> of 'o'?

It's not used, I've added it for symmetry, but we could drop it,
especially given that this is an internal API that we can easily
refactor. However, based on your comments below, this would need to be
turned into a

	_o<klass>();

so the argument will need to be kept. Maybe we should keep it for
LIBCAMERA_D_PTR() too, for symmetry.

> In general, I don't like much macros that defines variable, you use
> LIBCAMERA_O_PTR() and you have a variable 'o' declared. If you know
> the pattern that's fine. If you don't you'll see in code accesses to
> 'o' and you wonder where it is defined.

This bothered me too, I spent some time thinking about it and evaluating
alternatives when implementing this series. I didn't find any amazing
solution, but maybe I haven't looked hard enough.

> Can't this be used as (using patch 3/4 as an example)
>         'CameraManager * const cm = LIBCAMERA_O_PTR();

First of all, the const will be forgotten most of the time. Not
necessarily a big deal if the variable is declared manually though.

A second issue is that you'll have to add a const keyword in front of
CameraManager when calling this from const functions. The compiler will
generate an error if you don't, so that will be easy to catch. It will
cause a bit of time to be spent adding those const keywords that I'm
sure we'll also forget about, but that's probably not a big deal either.

> with
>         #define LIBCAMERA_O_PTR() _o()
> 
> Keep using the macro argument woult allow to remove the
> LIBCAMERA_DECLARE_PUBLIC() macro and turn the LIBCAMERA_O_PTR() into
> a direct access to o_ + static cast
> 
>         #define LIBCAMERA_O_PTR(klass) static_cast<klass>(o_)
>
> But then you probably need to provide a LIBCAMERA_CONST_O_PTR() which
> is not nice, unless the qualifiers can be transported with the 'klass'
> argument.

We will still need a friend statement in the private class to allow the
public class to access its private data and functions, so I think we
should keep LIBCAMERA_DECLARE_PUBLIC(). The _o() functions could however
be removed.

We don't need a LIBCAMERA_CONST_O_PTR(), as one could write

	const CameraManager *cm = LIBCAMERA_O_PTR(const CameraManager);

There is however one issue with const-correctness. The _o pointer stored
in Extensible::Private isn't const, so the following code would compile:

int CameraManager::Private::Foo() const
{
	CameraManager *cm = LIBCAMERA_O_PTR(CameraManager);
	...
}

This could be fixed by making _o private, and adding two accessors in
Extensible::Private

		Extensible *_o()
		{
			return o_;
		}

		const Extensible *_o() const
		{
			return o_;
		}

This could be solved by turning the _o function into a template
function. This is one of the reasons why _d() is a template function.
The other one was that _d() was initially a pair of functions in the
public class, defined by LIBCAMERA_DECLARE_PRIVATE() like the _o
functions are declared, but this caused an issue as the Private class
type wasn't fully defined, only declared, at the point where the code
had to be compiled. Templates help by delaying some of the evaluation by
the compiler.

> Another way to do is probably through templates like you do for d_ptr.
> 
> Anyway, my point is I would rather have an
>         ExtensibleDerivedClass *o = LIBCAMERA_SOME_MACRO(some arguments..)
> 
> Than having macros that defines variables..

One neat (in my opinion) feature of the macros as they are implemented
today is they enforce consistent naming of variables through the code
base. If we go for

	Private *d = LIBCAMERA_D_PTR(Public);

and

	Public *o = LIBCAMERA_O_PTR(Public);

I'll still want to enforce the variables being named d and o
respectively. Maybe that would be best handled by checkstyle.py than by
hiding the variable declarations in macros though. I'm interested in
hearing feedback from everybody on this topic.

> > +
> > +#else
> > +#define LIBCAMERA_DECLARE_PRIVATE(klass)
> > +#define LIBCAMERA_DECLARE_PUBLIC(klass)
> > +#define LIBCAMERA_D_PTR(klass)
> > +#define LIBCAMERA_O_PTR(klass)
> > +#endif
> > +
> > +class Extensible
> > +{
> > +public:
> > +	class Private
> > +	{
> > +	public:
> > +		Private(Extensible *o);
> > +		virtual ~Private();
> > +
> > +		Extensible * const o_;
> > +	};
> > +
> > +	Extensible(Private *d);
> > +
> > +protected:
> > +#ifndef __DOXYGEN__
> > +	template<typename T>
> > +	const T *_d() const
> > +	{
> > +		return static_cast<const T *>(d_.get());
> > +	}
> > +
> > +	template<typename T>
> > +	T *_d()
> > +	{
> > +		return static_cast<T*>(d_.get());
> > +	}
> > +#endif
> > +
> > +private:
> > +	const std::unique_ptr<Private> d_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_EXTENSIBLE_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index cdb8e0372e77..0a06dab0e105 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -8,6 +8,7 @@ libcamera_public_headers = files([
> >      'controls.h',
> >      'event_dispatcher.h',
> >      'event_notifier.h',
> > +    'extensible.h',
> >      'file_descriptor.h',
> >      'framebuffer_allocator.h',
> >      'geometry.h',
> > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp
> > new file mode 100644
> > index 000000000000..b2a7fc5d5304
> > --- /dev/null
> > +++ b/src/libcamera/extensible.cpp
> > @@ -0,0 +1,127 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * extensible.cpp - Utilities to create extensible public classes with stable ABIs
> > + */
> > +
> > +#include <libcamera/extensible.h>
> > +
> > +/**
> > + * \file extensible.h
> > + * \brief Utilities to create extensible public classes with stable ABIs
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \def LIBCAMERA_DECLARE_PRIVATE
> > + * \brief Declare private data for a public class
> > + * \param klass The public class name
> > + *
> > + * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to
> > + * make a class manage its private data through a d-pointer. It shall be used at
> > + * the very top of the class definition, with the public class name passed as
> > + * the \a klass parameter.
> > + */
> 
> Shall we estabilsh naming ? in this case you use 'public class', but I
> would rather uses 'extensible' for the visible one and 'hidden',
> 'private' or 'opaque' for the non-visible one ? I would establish a
> glossary at the beginning of documentation.

That's a good point, I think we should. I'll try to expand the
documentation.

> > +
> > +/**
> > + * \def LIBCAMERA_DECLARE_PUBLIC
> > + * \brief Declare public data for a private class
> > + * \param klass The public class name
> > + *
> > + * The LIBCAMERA_DECLARE_PUBLIC() macro is the counterpart of
> > + * LIBCAMERA_DECLARE_PRIVATE() to be used in the private data class. It shall be
> > + * used at the very top of the private class definition, with the public class
> > + * name passed as the \a klass parameter.
> 
> DECLARE_PUBLIC to be used in Private and DECLARE_PRIVATE to be used in
> Public. I know naming it's not easy...

I've thought about it as "declare the private class for this public
class" (and the other way around), but we could do the opposite if
that's preferred.

> > + */
> > +
> > +/**
> > + * \def LIBCAMERA_D_PTR(klass)
> > + * \brief Retrieve the private data pointer
> > + * \param[in] klass The public class name
> > + *
> > + * This macro can be used in any member function of a class that inherits,
> > + * directly or indirectly, from the Extensible class, to create a local
> > + * variable named 'd' that points to the class' private data instance.
> > + */
> > +
> > +/**
> > + * \def LIBCAMERA_O_PTR(klass)
> > + * \brief Retrieve the public instance corresponding to the private data
> > + * \param[in] klass The public class name
> > + *
> > + * This macro is the counterpart of LIBCAMERA_D_PTR() for private data classes.
> > + * It can be used in any member function of the private data class to create a
> > + * local variable named 'o' that ponits to the public class instance
> 
> s/ponits/points
> 
> > + * corresponding to the private data.
> > + */
> > +
> > +/**
> > + * \class Extensible
> > + * \brief Base class to manage private data through a d-pointer
> > + *
> > + * The Extensible class provides a base class to implement the
> > + * <a href="https://wiki.qt.io/D-Pointer">d-pointer</a> design pattern (also
> > + * known as <a href="https://en.wikipedia.org/wiki/Opaque_pointer">opaque pointer</a>
> > + * or <a href="https://en.cppreference.com/w/cpp/language/pimpl">pImpl idiom</a>).
> > + * It helps creating public classes that can be extended without breaking their
> > + * ABI. Such classes store their private data in a separate private data object,
> 
> You have used ABIs and ABI. Which one is correct ?

Both I think :-) Depending on the context, singular or plurar should be
used.

> > + * referenced by a pointer in the public class (hence the name d-pointer).
> > + *
> > + * Classes that follow this design pattern are referred herein as extensible
> > + * classes. To be extensible, a class PublicClass shall
> 
> shall: ?
> 
> > + *
> > + * - inherit from the Extensible class or from another extensible class
> > + * - invoke the LIBCAMERA_DECLARE_PRIVATE() macro at the very top of the class
> > + *   definition
> > + * - define a private data class named PublicClass::Private that inherits from
> > + *   the Private data class of the base class
> 
> "inherits from the Extensible::Private class." ?

No, because we could have multiple levels of inheritance, A could
inherit from B which inherits from Extensible. A::Private thus needs to
inherit from B::Private, not Extensible::Private.

> > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private
> > + *   data class definition
> > + * - pass a std::unique_ptr to a newly allocated Private data object to the
> > + *   constructor of the base class
> 
> The Extensible base class constructor does not take an unique_ptr, am
> I mistaken ?

Correct, I'll replace "std::unique_ptr" with "pointer".

> Also, shouldn't the PublicClass::Private derived class constructor call the
> PublicClass::Private() constructor with a pointer to the Extensible
> derived public class ?
> 
> Seems like a long review, but I like how it turned out!

Thank you :-)

> > + *
> > + * Additionally, if the PublicClass is not final, it shall expose one or more
> > + * constructors that takes a pointer to a Private data instance, to be used by
> > + * derived classes.
> > + */
> > +
> > +/**
> > + * \brief Construct an instance of an Extensible class
> > + * \param[in] d Pointer to the private data instance
> > + */
> > +Extensible::Extensible(Extensible::Private *d)
> > +	: d_(d)
> > +{
> > +}
> > +
> > +/**
> > + * \var Extensible::d_
> > + * \brief Pointer to the private data instance
> > + */
> > +
> > +/**
> > + * \class Extensible::Private
> > + * \brief Base class for private data managed through a d-pointer
> > + */
> > +
> > +/**
> > + * \brief Construct an instance of an Extensible class private data
> > + * \param[in] o Pointer to the public class object
> > + */
> > +Extensible::Private::Private(Extensible *o)
> > +	: o_(o)
> > +{
> > +}
> > +
> > +Extensible::Private::~Private()
> > +{
> > +}
> > +
> > +/**
> > + * \var Extensible::Private::o_
> > + * \brief Pointer to the public class object
> > + */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 0e6ecf5060a4..16c9c81440f5 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -16,6 +16,7 @@ libcamera_sources = files([
> >      'event_dispatcher.cpp',
> >      'event_dispatcher_poll.cpp',
> >      'event_notifier.cpp',
> > +    'extensible.cpp',
> >      'file.cpp',
> >      'file_descriptor.cpp',
> >      'formats.cpp',

-- 
Regards,

Laurent Pinchart