[libcamera-devel] [PATCH 1/4] libcamera: Add a base class to implement the d-pointer design pattern
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Oct 20 12:45:33 CEST 2020
Hello,
On 2020-10-20 04:02:31 +0300, Laurent Pinchart wrote:
> Hello,
>
> There's a question for everybody below.
>
> On Mon, Sep 21, 2020 at 02:55:28PM +0300, Laurent Pinchart wrote:
> > 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.
>
> Any feedback on this ? I've looked at pros and cons, and while I'm not a
> big fan of macros that hide variable creation, there are quite a few
> pros for doing so:
>
> - the variable naming will be consistent (could be caught by
> checkstyle.py alternatively)
> - the pointer will be const (could be caught by checkstyle.py
> alternatively)
> - the pointed data will automatically be const or mutable as required by
> the context (will be caught by the compiler otherwise)
>
> The drawbacks are that variable creation will be hidden, potentially
> conflicting with other local variables. However, if we want a consistent
> coding style, the names 'd' and 'o' should be reserved in Extensible
> classes anyway.
I agree with you, I'm no big fan of macros creating variables and it's a
short step to lc-moc ;-) But in this case I think it's acceptable. As
they will only be usable for the few object's we expose to applications.
And we already need to take so much care in these objects that assuming
specialise knowledge to work with them I would see as a plus.
I have no strong opinion tho and would not object to different
solutions.
>
> > > > +
> > > > +#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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list