[libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to implement the d-pointer design pattern
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 22 00:53:29 CEST 2020
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 at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list