[libcamera-devel,v2,2/5] libcamera: Add a base class to implement the d-pointer design pattern

10108 diff mbox series

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@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
Changes since v1:

- Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros
- Extend documentation
- Fix typos
---
 include/libcamera/extensible.h |  86 +++++++++++++++++++++
 include/libcamera/meson.build  |   1 +
 src/libcamera/extensible.cpp   | 134 +++++++++++++++++++++++++++++++++
 src/libcamera/meson.build      |   1 +
 4 files changed, 222 insertions(+)
 create mode 100644 include/libcamera/extensible.h
 create mode 100644 src/libcamera/extensible.cpp

Hi Laurent,

On Tue, Oct 20, 2020 at 04:40:02AM +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@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> Changes since v1:
>
> - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros
> - Extend documentation
> - Fix typos
> ---
>  include/libcamera/extensible.h |  86 +++++++++++++++++++++
>  include/libcamera/meson.build  |   1 +
>  src/libcamera/extensible.cpp   | 134 +++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build      |   1 +
>  4 files changed, 222 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..ea8808ad3e3c
> --- /dev/null
> +++ b/include/libcamera/extensible.h
> @@ -0,0 +1,86 @@
> +/* 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)					\
> +	friend class klass;

I am missing why this is needed.
It is meant to be used in klass::Private definition, but being Private
in the klass:: namespace, it shouldn't be necessary, right ?
(In facts, I removed it from Camera::Private and
CameraManager::Private from patches 4/5 and 5/5 and the compiler is
still happy.

> +
> +#define LIBCAMERA_D_PTR(klass)						\
> +	_d<klass::Private>();

Just <Private> and drop klass from arguments ?
Reading the documentation the first thing I wondered is "why do I need
to pass in the public class name to get a pointer to a private" ?

> +
> +#define LIBCAMERA_O_PTR(klass)						\
> +	_o<klass>();

I would rather provide two methods and let users call them without
bothering about the 'd' or 'o' name. If a class wants to call the
pointer to its private 'daffyduck' I don't see why we should force it
to be named 'd'.

In public class:

        Private *const abcd = _private();

In private class:
        ClassName *const asdsad = _public<ClassName>();

without introducing macros that has to be followed and really just
wrap one method call.

> +
> +#else
> +#define LIBCAMERA_DECLARE_PRIVATE(klass)
> +#define LIBCAMERA_DECLARE_PUBLIC(klass)
> +#define LIBCAMERA_D_PTR(klass)
> +#define LIBCAMERA_O_PTR(klass)
> +#endif

I wlso onder if we want macros, if LIBCAMERA_ is needed in the names
(and I won't question the decision to call 'D' pointer a
pointer to the internal Private class. I understand 'O' as it might
recall 'Outer', but 'D' ?)

Macro names with PRIVATE and PUBLIC are more expressive imo, but it
might be just a matter of getting used to it.

> +
> +class Extensible
> +{
> +public:
> +	class Private
> +	{
> +	public:
> +		Private(Extensible *o);
> +		virtual ~Private();
> +
> +#ifndef __DOXYGEN__
> +		template<typename T>
> +		const T *_o() const
> +		{
> +			return static_cast<const T *>(o_);
> +		}
> +
> +		template<typename T>
> +		T *_o()
> +		{
> +			return static_cast<T *>(o_);
> +		}
> +#endif
> +
> +	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 83bc46729314..15e6b43c9585 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',
>      'flags.h',
>      'framebuffer_allocator.h',
> diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp
> new file mode 100644
> index 000000000000..1dcb0bf1b12f
> --- /dev/null
> +++ b/src/libcamera/extensible.cpp
> @@ -0,0 +1,134 @@
> +/* 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.
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \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 points to the public class instance
> + * corresponding to the private data.

The two macros do not "create a local variable named ['o'|'d'] anymore

> + */
> +
> +/**
> + * \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,
> + * 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:
> + *
> + * - 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
> + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private
> + *   data class definition
> + * - pass a pointer to a newly allocated Private data object to the constructor
> + *   of the base class
> + *
> + * 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.
> + *
> + * The Private class is fully opaque to users of the libcamera public API.
> + * Internally, it can be kept private to the implementation of PublicClass, or
> + * be exposed to other classes. In the latter case, the members of the Private
> + * class need to be qualified with appropriate access specifiers. The
> + * PublicClass and Private classes always have full access to each other's
> + * protected and private members.
> + */
> +
> +/**
> + * \brief Construct an instance of an Extensible class
> + * \param[in] d Pointer to the private data instance
> + */
> +Extensible::Extensible(Extensible::Private *d)
> +	: d_(d)
> +{
> +}

I wonder if we could avoid hainvg in the extensible derived classes

        : Extensible(new Private(...))

by making Extensible accept a template argument pack that can be
forwarded to the construction of d_.

I just wonder, I'm not proposing to try it :)

> +
> +/**
> + * \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()

Why doesn't doxygen complain ?

Thanks
  j

> +{
> +}
> +
> +/**
> + * \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 47ddb4014a61..b9f6457433f9 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -17,6 +17,7 @@ libcamera_sources = files([
>      'event_dispatcher.cpp',
>      'event_dispatcher_poll.cpp',
>      'event_notifier.cpp',
> +    'extensible.cpp',
>      'file.cpp',
>      'file_descriptor.cpp',
>      'flags.cpp',
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 20, 2020 at 04:40:02AM +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@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > Changes since v1:
> >
> > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros
> > - Extend documentation
> > - Fix typos
> > ---
> >  include/libcamera/extensible.h |  86 +++++++++++++++++++++
> >  include/libcamera/meson.build  |   1 +
> >  src/libcamera/extensible.cpp   | 134 +++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build      |   1 +
> >  4 files changed, 222 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..ea8808ad3e3c
> > --- /dev/null
> > +++ b/include/libcamera/extensible.h
> > @@ -0,0 +1,86 @@
> > +/* 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)					\
> > +	friend class klass;
> 
> I am missing why this is needed.
> It is meant to be used in klass::Private definition, but being Private
> in the klass:: namespace, it shouldn't be necessary, right ?

It's meant to be used in the private class, yes. The macro name means
"declare the public class [for this private class]", hence usage of
LIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If
that's considered confusing, I'm fine switching the names, so that the
macro would mean "declare this class as the public class".

> (In facts, I removed it from Camera::Private and
> CameraManager::Private from patches 4/5 and 5/5 and the compiler is
> still happy.

That's because there's currently no private class that needs to accesses
fields from its public counterpart. I expect we'll need that later,
based on analysis of the d-pointer pattern implementation in Qt.

> > +
> > +#define LIBCAMERA_D_PTR(klass)						\
> > +	_d<klass::Private>();
> 
> Just <Private> and drop klass from arguments ?
> Reading the documentation the first thing I wondered is "why do I need
> to pass in the public class name to get a pointer to a private" ?

Dropping klass:: I can certainly do, but I think I'd prefer keeping
LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments
they take.

*However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()
as follows:

#define LIBCAMERA_DECLARE_PUBLIC(klass)					\
	friend class klass;						\
	using Public = klass;

then we could use _d<Public>() here, and drop the klass argument to both
LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?

I also had a look at how we could implement an outer_type_of<T> that
would resolve to U when T is U::Private. This would (I thinkg) allow
writing outer_type_of<decltype(*this)>. I haven't been able to find a
good way to do so.

> > +
> > +#define LIBCAMERA_O_PTR(klass)						\
> > +	_o<klass>();
> 
> I would rather provide two methods and let users call them without
> bothering about the 'd' or 'o' name. If a class wants to call the
> pointer to its private 'daffyduck' I don't see why we should force it
> to be named 'd'.
> 
> In public class:
> 
>         Private *const abcd = _private();

Note that this would need to be written _private<Private>().

> 
> In private class:
>         ClassName *const asdsad = _public<ClassName>();
> 
> without introducing macros that has to be followed and really just
> wrap one method call.

The variable name isn't constrained by this patch, unlike v1 that hid
the variable declaration in the macro. I however want to keep the names
consistent, as that increases readability of the code base. Anyone
familiar with libcamera should be able to immediately understand what d
and o are (d comes from the d-pointer design pattern and o stands for
object, but I'm fine discussing different names, especially for o),
hence patch 1/5 in this series to enforce that rule.

As for calling functions directly instead of using macros, I think the
macros make it more readable by hiding the implementation details.

> > +
> > +#else
> > +#define LIBCAMERA_DECLARE_PRIVATE(klass)
> > +#define LIBCAMERA_DECLARE_PUBLIC(klass)
> > +#define LIBCAMERA_D_PTR(klass)
> > +#define LIBCAMERA_O_PTR(klass)
> > +#endif
> 
> I wlso onder if we want macros, if LIBCAMERA_ is needed in the names
> (and I won't question the decision to call 'D' pointer a
> pointer to the internal Private class. I understand 'O' as it might
> recall 'Outer', but 'D' ?)

'd' comes from the name of the design pattern, called d-pointer. It's
also called pimpl (for pointer to implementation), but that's a horrible
name :-) As for 'o' (which I have meant as meaning object, but outer
seems better), I initially went for 'p', but that could be both public
or private, which isn't very nice. We could also have slightly longer
names if desired.

> Macro names with PRIVATE and PUBLIC are more expressive imo, but it
> might be just a matter of getting used to it.

The issue is that macros are not part of a namespace, so we need to make
sure they won't conflict with any third-party code we could use (or that
could be using us, as they're in a public header, but they could
possibly be moved to an internal header).

> > +
> > +class Extensible
> > +{
> > +public:
> > +	class Private
> > +	{
> > +	public:
> > +		Private(Extensible *o);
> > +		virtual ~Private();
> > +
> > +#ifndef __DOXYGEN__
> > +		template<typename T>
> > +		const T *_o() const
> > +		{
> > +			return static_cast<const T *>(o_);
> > +		}
> > +
> > +		template<typename T>
> > +		T *_o()
> > +		{
> > +			return static_cast<T *>(o_);
> > +		}
> > +#endif
> > +
> > +	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 83bc46729314..15e6b43c9585 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',
> >      'flags.h',
> >      'framebuffer_allocator.h',
> > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp
> > new file mode 100644
> > index 000000000000..1dcb0bf1b12f
> > --- /dev/null
> > +++ b/src/libcamera/extensible.cpp
> > @@ -0,0 +1,134 @@
> > +/* 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.
> > + */
> > +
> > +/**
> > + * \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.
> > + */
> > +
> > +/**
> > + * \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 points to the public class instance
> > + * corresponding to the private data.
> 
> The two macros do not "create a local variable named ['o'|'d'] anymore

Oops. I'll fix this (once we come to an agreement on the above
discussion).

> > + */
> > +
> > +/**
> > + * \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,
> > + * 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:
> > + *
> > + * - 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
> > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private
> > + *   data class definition
> > + * - pass a pointer to a newly allocated Private data object to the constructor
> > + *   of the base class
> > + *
> > + * 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.
> > + *
> > + * The Private class is fully opaque to users of the libcamera public API.
> > + * Internally, it can be kept private to the implementation of PublicClass, or
> > + * be exposed to other classes. In the latter case, the members of the Private
> > + * class need to be qualified with appropriate access specifiers. The
> > + * PublicClass and Private classes always have full access to each other's
> > + * protected and private members.
> > + */
> > +
> > +/**
> > + * \brief Construct an instance of an Extensible class
> > + * \param[in] d Pointer to the private data instance
> > + */
> > +Extensible::Extensible(Extensible::Private *d)
> > +	: d_(d)
> > +{
> > +}
> 
> I wonder if we could avoid hainvg in the extensible derived classes
> 
>         : Extensible(new Private(...))
> 
> by making Extensible accept a template argument pack that can be
> forwarded to the construction of d_.
> 
> I just wonder, I'm not proposing to try it :)

I'm not sure to follow you, do you have an example ?

> > +
> > +/**
> > + * \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()
> 
> Why doesn't doxygen complain ?

Because it contains this:

EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
                         libcamera::BoundMethodBase \
                         libcamera::BoundMethodMember \
                         libcamera::BoundMethodPack \
                         libcamera::BoundMethodPackBase \
                         libcamera::BoundMethodStatic \
                         libcamera::SignalBase \
                         libcamera::*::Private \
                         *::details::* \
                         std::*

> > +{
> > +}
> > +
> > +/**
> > + * \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 47ddb4014a61..b9f6457433f9 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -17,6 +17,7 @@ libcamera_sources = files([
> >      'event_dispatcher.cpp',
> >      'event_dispatcher_poll.cpp',
> >      'event_notifier.cpp',
> > +    'extensible.cpp',
> >      'file.cpp',
> >      'file_descriptor.cpp',
> >      'flags.cpp',

Hi Laurent,

On Thu, Oct 22, 2020 at 01:53:29AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 20, 2020 at 04:40:02AM +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@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > > Changes since v1:
> > >
> > > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros
> > > - Extend documentation
> > > - Fix typos
> > > ---
> > >  include/libcamera/extensible.h |  86 +++++++++++++++++++++
> > >  include/libcamera/meson.build  |   1 +
> > >  src/libcamera/extensible.cpp   | 134 +++++++++++++++++++++++++++++++++
> > >  src/libcamera/meson.build      |   1 +
> > >  4 files changed, 222 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..ea8808ad3e3c
> > > --- /dev/null
> > > +++ b/include/libcamera/extensible.h
> > > @@ -0,0 +1,86 @@
> > > +/* 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)					\
> > > +	friend class klass;
> >
> > I am missing why this is needed.
> > It is meant to be used in klass::Private definition, but being Private
> > in the klass:: namespace, it shouldn't be necessary, right ?
>
> It's meant to be used in the private class, yes. The macro name means
> "declare the public class [for this private class]", hence usage of
> LIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If
> that's considered confusing, I'm fine switching the names, so that the
> macro would mean "declare this class as the public class".
>

I meant I was not sure why the 'friend' declaration is required, as
the Private class is declared inside the Public one and thus should be
accessible

> > (In facts, I removed it from Camera::Private and
> > CameraManager::Private from patches 4/5 and 5/5 and the compiler is
> > still happy.
>
> That's because there's currently no private class that needs to accesses
> fields from its public counterpart. I expect we'll need that later,
> based on analysis of the d-pointer pattern implementation in Qt.
>

I see.

My doubt on Private being declared inside Public thus not requiring
the "friend class klass" has been clarified by the compiler
complaining because of

--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -36,8 +36,6 @@ LOG_DEFINE_CATEGORY(Camera)

 class CameraManager::Private : public Extensible::Private, public Thread
 {
-       LIBCAMERA_DECLARE_PUBLIC(CameraManager)
-
 public:
        Private(CameraManager *cm);

@@ -72,6 +70,8 @@ private:

        IPAManager ipaManager_;
        ProcessManager processManager_;
+
+       int a;
 };

 CameraManager::Private::Private(CameraManager *cm)
@@ -296,6 +296,9 @@ int CameraManager::start()
 {
        Private *const d = LIBCAMERA_D_PTR(CameraManager);

+       if (d->a)
+               return 0;
+
        LOG(Camera, Info) << "libcamera " << version_;

        int ret = d->start();

> > > +
> > > +#define LIBCAMERA_D_PTR(klass)						\
> > > +	_d<klass::Private>();
> >
> > Just <Private> and drop klass from arguments ?
> > Reading the documentation the first thing I wondered is "why do I need
> > to pass in the public class name to get a pointer to a private" ?
>
> Dropping klass:: I can certainly do, but I think I'd prefer keeping
> LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments
> they take.
>
> *However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()
> as follows:
>
> #define LIBCAMERA_DECLARE_PUBLIC(klass)					\
> 	friend class klass;						\
> 	using Public = klass;
>
> then we could use _d<Public>() here, and drop the klass argument to both
> LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?

I like it better, but if you prefer symetrical macros, that's fine too

>
> I also had a look at how we could implement an outer_type_of<T> that
> would resolve to U when T is U::Private. This would (I thinkg) allow
> writing outer_type_of<decltype(*this)>. I haven't been able to find a
> good way to do so.
>

Not following you down this rabbit hole, I'm sorry :)

> > > +
> > > +#define LIBCAMERA_O_PTR(klass)						\
> > > +	_o<klass>();
> >
> > I would rather provide two methods and let users call them without
> > bothering about the 'd' or 'o' name. If a class wants to call the
> > pointer to its private 'daffyduck' I don't see why we should force it
> > to be named 'd'.
> >
> > In public class:
> >
> >         Private *const abcd = _private();
>
> Note that this would need to be written _private<Private>().
>
> >
> > In private class:
> >         ClassName *const asdsad = _public<ClassName>();
> >
> > without introducing macros that has to be followed and really just
> > wrap one method call.
>
> The variable name isn't constrained by this patch, unlike v1 that hid
> the variable declaration in the macro. I however want to keep the names
> consistent, as that increases readability of the code base. Anyone
> familiar with libcamera should be able to immediately understand what d
> and o are (d comes from the d-pointer design pattern and o stands for
> object, but I'm fine discussing different names, especially for o),
> hence patch 1/5 in this series to enforce that rule.

Yeah, I meant not reporting an error in checkstyle.

>
> As for calling functions directly instead of using macros, I think the
> macros make it more readable by hiding the implementation details.
>

Ok, so, and sorry for backtracking, if you want 'd' and 'o' to be
forced and get it as recognizible libcamera construct, macros are fine
too

> > > +
> > > +#else
> > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)
> > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)
> > > +#define LIBCAMERA_D_PTR(klass)
> > > +#define LIBCAMERA_O_PTR(klass)
> > > +#endif
> >
> > I wlso onder if we want macros, if LIBCAMERA_ is needed in the names
> > (and I won't question the decision to call 'D' pointer a
> > pointer to the internal Private class. I understand 'O' as it might
> > recall 'Outer', but 'D' ?)
>
> 'd' comes from the name of the design pattern, called d-pointer. It's
> also called pimpl (for pointer to implementation), but that's a horrible
> name :-) As for 'o' (which I have meant as meaning object, but outer
> seems better), I initially went for 'p', but that could be both public
> or private, which isn't very nice. We could also have slightly longer
> names if desired.
>
> > Macro names with PRIVATE and PUBLIC are more expressive imo, but it
> > might be just a matter of getting used to it.
>
> The issue is that macros are not part of a namespace, so we need to make
> sure they won't conflict with any third-party code we could use (or that
> could be using us, as they're in a public header, but they could
> possibly be moved to an internal header).

I'm fine keeping LIBCAMERA_ then

>
> > > +
> > > +class Extensible
> > > +{
> > > +public:
> > > +	class Private
> > > +	{
> > > +	public:
> > > +		Private(Extensible *o);
> > > +		virtual ~Private();
> > > +
> > > +#ifndef __DOXYGEN__
> > > +		template<typename T>
> > > +		const T *_o() const
> > > +		{
> > > +			return static_cast<const T *>(o_);
> > > +		}
> > > +
> > > +		template<typename T>
> > > +		T *_o()
> > > +		{
> > > +			return static_cast<T *>(o_);
> > > +		}
> > > +#endif
> > > +
> > > +	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 83bc46729314..15e6b43c9585 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',
> > >      'flags.h',
> > >      'framebuffer_allocator.h',
> > > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp
> > > new file mode 100644
> > > index 000000000000..1dcb0bf1b12f
> > > --- /dev/null
> > > +++ b/src/libcamera/extensible.cpp
> > > @@ -0,0 +1,134 @@
> > > +/* 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.
> > > + */
> > > +
> > > +/**
> > > + * \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.
> > > + */
> > > +
> > > +/**
> > > + * \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 points to the public class instance
> > > + * corresponding to the private data.
> >
> > The two macros do not "create a local variable named ['o'|'d'] anymore
>
> Oops. I'll fix this (once we come to an agreement on the above
> discussion).
>
> > > + */
> > > +
> > > +/**
> > > + * \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,
> > > + * 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:
> > > + *
> > > + * - 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
> > > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private
> > > + *   data class definition
> > > + * - pass a pointer to a newly allocated Private data object to the constructor
> > > + *   of the base class
> > > + *
> > > + * 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.
> > > + *
> > > + * The Private class is fully opaque to users of the libcamera public API.
> > > + * Internally, it can be kept private to the implementation of PublicClass, or
> > > + * be exposed to other classes. In the latter case, the members of the Private
> > > + * class need to be qualified with appropriate access specifiers. The
> > > + * PublicClass and Private classes always have full access to each other's
> > > + * protected and private members.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct an instance of an Extensible class
> > > + * \param[in] d Pointer to the private data instance
> > > + */
> > > +Extensible::Extensible(Extensible::Private *d)
> > > +	: d_(d)
> > > +{
> > > +}
> >
> > I wonder if we could avoid hainvg in the extensible derived classes
> >
> >         : Extensible(new Private(...))
> >
> > by making Extensible accept a template argument pack that can be
> > forwarded to the construction of d_.
> >
> > I just wonder, I'm not proposing to try it :)
>
> I'm not sure to follow you, do you have an example ?
>

Don't worry, I was just thinking out loud. Please ignore this.

> > > +
> > > +/**
> > > + * \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()
> >
> > Why doesn't doxygen complain ?
>
> Because it contains this:
>
> EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
>                          libcamera::BoundMethodBase \
>                          libcamera::BoundMethodMember \
>                          libcamera::BoundMethodPack \
>                          libcamera::BoundMethodPackBase \
>                          libcamera::BoundMethodStatic \
>                          libcamera::SignalBase \
>                          libcamera::*::Private \
>                          *::details::* \
>                          std::*

Ack.

Overall I think I've pestered you enough. If you want to enforceusage
of 'd' and 'o' having macros to either declare the variable or just shorten
the access is fine. I still lean towards letting classes open code
access to private and public classes, without enforcing naming, but
everything is fine.

For next version, with minors fixed:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \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 47ddb4014a61..b9f6457433f9 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -17,6 +17,7 @@ libcamera_sources = files([
> > >      'event_dispatcher.cpp',
> > >      'event_dispatcher_poll.cpp',
> > >      'event_notifier.cpp',
> > > +    'extensible.cpp',
> > >      'file.cpp',
> > >      'file_descriptor.cpp',
> > >      'flags.cpp',
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Tue, Oct 27, 2020 at 01:11:06PM +0100, Jacopo Mondi wrote:
> On Thu, Oct 22, 2020 at 01:53:29AM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:
> > > On Tue, Oct 20, 2020 at 04:40:02AM +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@ideasonboard.com>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros
> > > > - Extend documentation
> > > > - Fix typos
> > > > ---
> > > >  include/libcamera/extensible.h |  86 +++++++++++++++++++++
> > > >  include/libcamera/meson.build  |   1 +
> > > >  src/libcamera/extensible.cpp   | 134 +++++++++++++++++++++++++++++++++
> > > >  src/libcamera/meson.build      |   1 +
> > > >  4 files changed, 222 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..ea8808ad3e3c
> > > > --- /dev/null
> > > > +++ b/include/libcamera/extensible.h
> > > > @@ -0,0 +1,86 @@
> > > > +/* 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)					\
> > > > +	friend class klass;
> > >
> > > I am missing why this is needed.
> > > It is meant to be used in klass::Private definition, but being Private
> > > in the klass:: namespace, it shouldn't be necessary, right ?
> >
> > It's meant to be used in the private class, yes. The macro name means
> > "declare the public class [for this private class]", hence usage of
> > LIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If
> > that's considered confusing, I'm fine switching the names, so that the
> > macro would mean "declare this class as the public class".
> 
> I meant I was not sure why the 'friend' declaration is required, as
> the Private class is declared inside the Public one and thus should be
> accessible

class Public
{ 
public:
	Public();

private:
	class Private
	{
	public:
		Private();
	private:
		int data_;
	};

	Private *private_;
};

Public::Public()
{
	private_ = new Private();
	private_->data_ = 0;
}

private.cpp: In constructor ‘Public::Public()’:
private.cpp:21:12: error: ‘int Public::Private::data_’ is private within this context
  private_->data_ = 0;
            ^~~~~
private.cpp:12:7: note: declared private here
   int data_;
       ^~~~~

> > > (In facts, I removed it from Camera::Private and
> > > CameraManager::Private from patches 4/5 and 5/5 and the compiler is
> > > still happy.
> >
> > That's because there's currently no private class that needs to accesses
> > fields from its public counterpart. I expect we'll need that later,
> > based on analysis of the d-pointer pattern implementation in Qt.
> 
> I see.
> 
> My doubt on Private being declared inside Public thus not requiring
> the "friend class klass" has been clarified by the compiler
> complaining because of
> 
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -36,8 +36,6 @@ LOG_DEFINE_CATEGORY(Camera)
> 
>  class CameraManager::Private : public Extensible::Private, public Thread
>  {
> -       LIBCAMERA_DECLARE_PUBLIC(CameraManager)
> -
>  public:
>         Private(CameraManager *cm);
> 
> @@ -72,6 +70,8 @@ private:
> 
>         IPAManager ipaManager_;
>         ProcessManager processManager_;
> +
> +       int a;
>  };
> 
>  CameraManager::Private::Private(CameraManager *cm)
> @@ -296,6 +296,9 @@ int CameraManager::start()
>  {
>         Private *const d = LIBCAMERA_D_PTR(CameraManager);
> 
> +       if (d->a)
> +               return 0;
> +
>         LOG(Camera, Info) << "libcamera " << version_;
> 
>         int ret = d->start();

Right, we're tested the same thing :-)

> > > > +
> > > > +#define LIBCAMERA_D_PTR(klass)						\
> > > > +	_d<klass::Private>();
> > >
> > > Just <Private> and drop klass from arguments ?
> > > Reading the documentation the first thing I wondered is "why do I need
> > > to pass in the public class name to get a pointer to a private" ?
> >
> > Dropping klass:: I can certainly do, but I think I'd prefer keeping
> > LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments
> > they take.
> >
> > *However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()
> > as follows:
> >
> > #define LIBCAMERA_DECLARE_PUBLIC(klass)					\
> > 	friend class klass;						\
> > 	using Public = klass;
> >
> > then we could use _d<Public>() here, and drop the klass argument to both
> > LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?
> 
> I like it better, but if you prefer symetrical macros, that's fine too

I'd prefer LIBCAMERA_DECLARE_PUBLIC() and LIBCAMERA_DECLARE_PRIVATE() to
be symmetrical indeed, but we can at least drop the klass argument to
LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR().

> > I also had a look at how we could implement an outer_type_of<T> that
> > would resolve to U when T is U::Private. This would (I thinkg) allow
> > writing outer_type_of<decltype(*this)>. I haven't been able to find a
> > good way to do so.
> 
> Not following you down this rabbit hole, I'm sorry :)

:-)

> > > > +
> > > > +#define LIBCAMERA_O_PTR(klass)						\
> > > > +	_o<klass>();
> > >
> > > I would rather provide two methods and let users call them without
> > > bothering about the 'd' or 'o' name. If a class wants to call the
> > > pointer to its private 'daffyduck' I don't see why we should force it
> > > to be named 'd'.
> > >
> > > In public class:
> > >
> > >         Private *const abcd = _private();
> >
> > Note that this would need to be written _private<Private>().
> >
> > >
> > > In private class:
> > >         ClassName *const asdsad = _public<ClassName>();
> > >
> > > without introducing macros that has to be followed and really just
> > > wrap one method call.
> >
> > The variable name isn't constrained by this patch, unlike v1 that hid
> > the variable declaration in the macro. I however want to keep the names
> > consistent, as that increases readability of the code base. Anyone
> > familiar with libcamera should be able to immediately understand what d
> > and o are (d comes from the d-pointer design pattern and o stands for
> > object, but I'm fine discussing different names, especially for o),
> > hence patch 1/5 in this series to enforce that rule.
> 
> Yeah, I meant not reporting an error in checkstyle.
> 
> > As for calling functions directly instead of using macros, I think the
> > macros make it more readable by hiding the implementation details.
> 
> Ok, so, and sorry for backtracking, if you want 'd' and 'o' to be
> forced and get it as recognizible libcamera construct, macros are fine
> too

Just to be sure, does this mean you prefer

	Private * const d = LIBCAMERA_D_PTR();

or

	LIBCAMERA_D_PTR();

with the d variable hidden in the macro ?

> > > > +
> > > > +#else
> > > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)
> > > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)
> > > > +#define LIBCAMERA_D_PTR(klass)
> > > > +#define LIBCAMERA_O_PTR(klass)
> > > > +#endif
> > >
> > > I wlso onder if we want macros, if LIBCAMERA_ is needed in the names
> > > (and I won't question the decision to call 'D' pointer a
> > > pointer to the internal Private class. I understand 'O' as it might
> > > recall 'Outer', but 'D' ?)
> >
> > 'd' comes from the name of the design pattern, called d-pointer. It's
> > also called pimpl (for pointer to implementation), but that's a horrible
> > name :-) As for 'o' (which I have meant as meaning object, but outer
> > seems better), I initially went for 'p', but that could be both public
> > or private, which isn't very nice. We could also have slightly longer
> > names if desired.
> >
> > > Macro names with PRIVATE and PUBLIC are more expressive imo, but it
> > > might be just a matter of getting used to it.
> >
> > The issue is that macros are not part of a namespace, so we need to make
> > sure they won't conflict with any third-party code we could use (or that
> > could be using us, as they're in a public header, but they could
> > possibly be moved to an internal header).
> 
> I'm fine keeping LIBCAMERA_ then
> 
> > > > +
> > > > +class Extensible
> > > > +{
> > > > +public:
> > > > +	class Private
> > > > +	{
> > > > +	public:
> > > > +		Private(Extensible *o);
> > > > +		virtual ~Private();
> > > > +
> > > > +#ifndef __DOXYGEN__
> > > > +		template<typename T>
> > > > +		const T *_o() const
> > > > +		{
> > > > +			return static_cast<const T *>(o_);
> > > > +		}
> > > > +
> > > > +		template<typename T>
> > > > +		T *_o()
> > > > +		{
> > > > +			return static_cast<T *>(o_);
> > > > +		}
> > > > +#endif
> > > > +
> > > > +	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 83bc46729314..15e6b43c9585 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',
> > > >      'flags.h',
> > > >      'framebuffer_allocator.h',
> > > > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp
> > > > new file mode 100644
> > > > index 000000000000..1dcb0bf1b12f
> > > > --- /dev/null
> > > > +++ b/src/libcamera/extensible.cpp
> > > > @@ -0,0 +1,134 @@
> > > > +/* 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.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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 points to the public class instance
> > > > + * corresponding to the private data.
> > >
> > > The two macros do not "create a local variable named ['o'|'d'] anymore
> >
> > Oops. I'll fix this (once we come to an agreement on the above
> > discussion).
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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,
> > > > + * 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:
> > > > + *
> > > > + * - 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
> > > > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private
> > > > + *   data class definition
> > > > + * - pass a pointer to a newly allocated Private data object to the constructor
> > > > + *   of the base class
> > > > + *
> > > > + * 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.
> > > > + *
> > > > + * The Private class is fully opaque to users of the libcamera public API.
> > > > + * Internally, it can be kept private to the implementation of PublicClass, or
> > > > + * be exposed to other classes. In the latter case, the members of the Private
> > > > + * class need to be qualified with appropriate access specifiers. The
> > > > + * PublicClass and Private classes always have full access to each other's
> > > > + * protected and private members.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Construct an instance of an Extensible class
> > > > + * \param[in] d Pointer to the private data instance
> > > > + */
> > > > +Extensible::Extensible(Extensible::Private *d)
> > > > +	: d_(d)
> > > > +{
> > > > +}
> > >
> > > I wonder if we could avoid hainvg in the extensible derived classes
> > >
> > >         : Extensible(new Private(...))
> > >
> > > by making Extensible accept a template argument pack that can be
> > > forwarded to the construction of d_.
> > >
> > > I just wonder, I'm not proposing to try it :)
> >
> > I'm not sure to follow you, do you have an example ?
> 
> Don't worry, I was just thinking out loud. Please ignore this.
> 
> > > > +
> > > > +/**
> > > > + * \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()
> > >
> > > Why doesn't doxygen complain ?
> >
> > Because it contains this:
> >
> > EXCLUDE_SYMBOLS        = libcamera::BoundMethodArgs \
> >                          libcamera::BoundMethodBase \
> >                          libcamera::BoundMethodMember \
> >                          libcamera::BoundMethodPack \
> >                          libcamera::BoundMethodPackBase \
> >                          libcamera::BoundMethodStatic \
> >                          libcamera::SignalBase \
> >                          libcamera::*::Private \
> >                          *::details::* \
> >                          std::*
> 
> Ack.
> 
> Overall I think I've pestered you enough. If you want to enforce usage
> of 'd' and 'o' having macros to either declare the variable or just shorten
> the access is fine. I still lean towards letting classes open code
> access to private and public classes, without enforcing naming, but
> everything is fine.
> 
> For next version, with minors fixed:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thank you :-)

> > > > +{
> > > > +}
> > > > +
> > > > +/**
> > > > + * \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 47ddb4014a61..b9f6457433f9 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -17,6 +17,7 @@ libcamera_sources = files([
> > > >      'event_dispatcher.cpp',
> > > >      'event_dispatcher_poll.cpp',
> > > >      'event_notifier.cpp',
> > > > +    'extensible.cpp',
> > > >      'file.cpp',
> > > >      'file_descriptor.cpp',
> > > >      'flags.cpp',

Hi Laurent

On Tue, Oct 27, 2020 at 02:19:15PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Oct 27, 2020 at 01:11:06PM +0100, Jacopo Mondi wrote:

[snip]

> > > > In private class:
> > > >         ClassName *const asdsad = _public<ClassName>();
> > > >
> > > > without introducing macros that has to be followed and really just
> > > > wrap one method call.
> > >
> > > The variable name isn't constrained by this patch, unlike v1 that hid
> > > the variable declaration in the macro. I however want to keep the names
> > > consistent, as that increases readability of the code base. Anyone
> > > familiar with libcamera should be able to immediately understand what d
> > > and o are (d comes from the d-pointer design pattern and o stands for
> > > object, but I'm fine discussing different names, especially for o),
> > > hence patch 1/5 in this series to enforce that rule.
> >
> > Yeah, I meant not reporting an error in checkstyle.
> >
> > > As for calling functions directly instead of using macros, I think the
> > > macros make it more readable by hiding the implementation details.
> >
> > Ok, so, and sorry for backtracking, if you want 'd' and 'o' to be
> > forced and get it as recognizible libcamera construct, macros are fine
> > too
>
> Just to be sure, does this mean you prefer
>
> 	Private * const d = LIBCAMERA_D_PTR();
>
> or
>
> 	LIBCAMERA_D_PTR();
>
> with the d variable hidden in the macro ?

I would prefer letting each implementation use the variable name they
like, without introducing any library specific pattern that increase
the entry barrier to grasp the code base.

But I see your point and if you want to force the usage of the 'o' and
'd' variable names using checkstyle, I think the two are equivalent,
with a slight preference for the first that at least has the variable
name explicit and not hidden behind a macro.

Up to you, really.