Message ID | 20201020014005.12783-3-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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.
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; + +#define LIBCAMERA_D_PTR(klass) \ + _d<klass::Private>(); + +#define LIBCAMERA_O_PTR(klass) \ + _o<klass>(); + +#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(); + +#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. + */ + +/** + * \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) +{ +} + +/** + * \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 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',