[libcamera-devel] [PATCH 1/4] libcamera: Add a base class to implement the d-pointer design pattern
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 21 11:25:48 CEST 2020
Hi Laurent,
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 ?
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 ?
> +
> +#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'?
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.
Can't this be used as (using patch 3/4 as an example)
'CameraManager * const cm = LIBCAMERA_O_PTR();
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.
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..
> +
> +#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.
> +
> +/**
> + * \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...
> + */
> +
> +/**
> + * \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 ?
> + * 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." ?
> + * - 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 ?
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!
Thanks
j
> + *
> + * 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list