[{"id":13319,"web_url":"https://patchwork.libcamera.org/comment/13319/","msgid":"<20201021100412.2c572pwyw4xntsoi@uno.localdomain>","date":"2020-10-21T10:04:12","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 20, 2020 at 04:40:02AM +0300, Laurent Pinchart wrote:\n> The d-pointer design patterns helps creating public classes that can be\n> extended without breaking their ABI. To facilitate usage of the pattern\n> in libcamera, create a base Extensible class with associated macros.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Niklas Söderlund \n> ---\n> Changes since v1:\n>\n> - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros\n> - Extend documentation\n> - Fix typos\n> ---\n> include/libcamera/extensible.h | 86 +++++++++++++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/extensible.cpp | 134 +++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 222 insertions(+)\n> create mode 100644 include/libcamera/extensible.h\n> create mode 100644 src/libcamera/extensible.cpp\n>\n> diff --git a/include/libcamera/extensible.h b/include/libcamera/extensible.h\n> new file mode 100644\n> index 000000000000..ea8808ad3e3c\n> --- /dev/null\n> +++ b/include/libcamera/extensible.h\n> @@ -0,0 +1,86 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * extensible.h - Utilities to create extensible public classes with stable ABIs\n> + */\n> +#ifndef __LIBCAMERA_EXTENSIBLE_H__\n> +#define __LIBCAMERA_EXTENSIBLE_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +#ifndef __DOXYGEN__\n> +#define LIBCAMERA_DECLARE_PRIVATE(klass)\t\t\t\t\\\n> +public:\t\t\t\t\t\t\t\t\t\\\n> +\tclass Private;\t\t\t\t\t\t\t\\\n> +\tfriend class Private;\n> +\n> +#define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> +\tfriend class klass;\n\nI am missing why this is needed.\nIt is meant to be used in klass::Private definition, but being Private\nin the klass:: namespace, it shouldn't be necessary, right ?\n(In facts, I removed it from Camera::Private and\nCameraManager::Private from patches 4/5 and 5/5 and the compiler is\nstill happy.\n\n> +\n> +#define LIBCAMERA_D_PTR(klass)\t\t\t\t\t\t\\\n> +\t_d();\n\nJust and drop klass from arguments ?\nReading the documentation the first thing I wondered is \"why do I need\nto pass in the public class name to get a pointer to a private\" ?\n\n> +\n> +#define LIBCAMERA_O_PTR(klass)\t\t\t\t\t\t\\\n> +\t_o();\n\nI would rather provide two methods and let users call them without\nbothering about the 'd' or 'o' name. If a class wants to call the\npointer to its private 'daffyduck' I don't see why we should force it\nto be named 'd'.\n\nIn public class:\n\n Private *const abcd = _private();\n\nIn private class:\n ClassName *const asdsad = _public();\n\nwithout introducing macros that has to be followed and really just\nwrap one method call.\n\n> +\n> +#else\n> +#define LIBCAMERA_DECLARE_PRIVATE(klass)\n> +#define LIBCAMERA_DECLARE_PUBLIC(klass)\n> +#define LIBCAMERA_D_PTR(klass)\n> +#define LIBCAMERA_O_PTR(klass)\n> +#endif\n\nI wlso onder if we want macros, if LIBCAMERA_ is needed in the names\n(and I won't question the decision to call 'D' pointer a\npointer to the internal Private class. I understand 'O' as it might\nrecall 'Outer', but 'D' ?)\n\nMacro names with PRIVATE and PUBLIC are more expressive imo, but it\nmight be just a matter of getting used to it.\n\n> +\n> +class Extensible\n> +{\n> +public:\n> +\tclass Private\n> +\t{\n> +\tpublic:\n> +\t\tPrivate(Extensible *o);\n> +\t\tvirtual ~Private();\n> +\n> +#ifndef __DOXYGEN__\n> +\t\ttemplate\n> +\t\tconst T *_o() const\n> +\t\t{\n> +\t\t\treturn static_cast(o_);\n> +\t\t}\n> +\n> +\t\ttemplate\n> +\t\tT *_o()\n> +\t\t{\n> +\t\t\treturn static_cast(o_);\n> +\t\t}\n> +#endif\n> +\n> +\tprivate:\n> +\t\tExtensible * const o_;\n> +\t};\n> +\n> +\tExtensible(Private *d);\n> +\n> +protected:\n> +#ifndef __DOXYGEN__\n> +\ttemplate\n> +\tconst T *_d() const\n> +\t{\n> +\t\treturn static_cast(d_.get());\n> +\t}\n> +\n> +\ttemplate\n> +\tT *_d()\n> +\t{\n> +\t\treturn static_cast(d_.get());\n> +\t}\n> +#endif\n> +\n> +private:\n> +\tconst std::unique_ptr d_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_EXTENSIBLE_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 83bc46729314..15e6b43c9585 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -8,6 +8,7 @@ libcamera_public_headers = files([\n> 'controls.h',\n> 'event_dispatcher.h',\n> 'event_notifier.h',\n> + 'extensible.h',\n> 'file_descriptor.h',\n> 'flags.h',\n> 'framebuffer_allocator.h',\n> diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp\n> new file mode 100644\n> index 000000000000..1dcb0bf1b12f\n> --- /dev/null\n> +++ b/src/libcamera/extensible.cpp\n> @@ -0,0 +1,134 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * extensible.cpp - Utilities to create extensible public classes with stable ABIs\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file extensible.h\n> + * \\brief Utilities to create extensible public classes with stable ABIs\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\def LIBCAMERA_DECLARE_PRIVATE\n> + * \\brief Declare private data for a public class\n> + * \\param klass The public class name\n> + *\n> + * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to\n> + * make a class manage its private data through a d-pointer. It shall be used at\n> + * the very top of the class definition, with the public class name passed as\n> + * the \\a klass parameter.\n> + */\n> +\n> +/**\n> + * \\def LIBCAMERA_DECLARE_PUBLIC\n> + * \\brief Declare public data for a private class\n> + * \\param klass The public class name\n> + *\n> + * The LIBCAMERA_DECLARE_PUBLIC() macro is the counterpart of\n> + * LIBCAMERA_DECLARE_PRIVATE() to be used in the private data class. It shall be\n> + * used at the very top of the private class definition, with the public class\n> + * name passed as the \\a klass parameter.\n> + */\n> +\n> +/**\n> + * \\def LIBCAMERA_D_PTR(klass)\n> + * \\brief Retrieve the private data pointer\n> + * \\param[in] klass The public class name\n> + *\n> + * This macro can be used in any member function of a class that inherits,\n> + * directly or indirectly, from the Extensible class, to create a local\n> + * variable named 'd' that points to the class' private data instance.\n> + */\n> +\n> +/**\n> + * \\def LIBCAMERA_O_PTR(klass)\n> + * \\brief Retrieve the public instance corresponding to the private data\n> + * \\param[in] klass The public class name\n> + *\n> + * This macro is the counterpart of LIBCAMERA_D_PTR() for private data classes.\n> + * It can be used in any member function of the private data class to create a\n> + * local variable named 'o' that points to the public class instance\n> + * corresponding to the private data.\n\nThe two macros do not \"create a local variable named ['o'|'d'] anymore\n\n> + */\n> +\n> +/**\n> + * \\class Extensible\n> + * \\brief Base class to manage private data through a d-pointer\n> + *\n> + * The Extensible class provides a base class to implement the\n> + * d-pointer design pattern (also\n> + * known as opaque pointer\n> + * or pImpl idiom).\n> + * It helps creating public classes that can be extended without breaking their\n> + * ABI. Such classes store their private data in a separate private data object,\n> + * referenced by a pointer in the public class (hence the name d-pointer).\n> + *\n> + * Classes that follow this design pattern are referred herein as extensible\n> + * classes. To be extensible, a class PublicClass shall:\n> + *\n> + * - inherit from the Extensible class or from another extensible class\n> + * - invoke the LIBCAMERA_DECLARE_PRIVATE() macro at the very top of the class\n> + * definition\n> + * - define a private data class named PublicClass::Private that inherits from\n> + * the Private data class of the base class\n> + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private\n> + * data class definition\n> + * - pass a pointer to a newly allocated Private data object to the constructor\n> + * of the base class\n> + *\n> + * Additionally, if the PublicClass is not final, it shall expose one or more\n> + * constructors that takes a pointer to a Private data instance, to be used by\n> + * derived classes.\n> + *\n> + * The Private class is fully opaque to users of the libcamera public API.\n> + * Internally, it can be kept private to the implementation of PublicClass, or\n> + * be exposed to other classes. In the latter case, the members of the Private\n> + * class need to be qualified with appropriate access specifiers. The\n> + * PublicClass and Private classes always have full access to each other's\n> + * protected and private members.\n> + */\n> +\n> +/**\n> + * \\brief Construct an instance of an Extensible class\n> + * \\param[in] d Pointer to the private data instance\n> + */\n> +Extensible::Extensible(Extensible::Private *d)\n> +\t: d_(d)\n> +{\n> +}\n\nI wonder if we could avoid hainvg in the extensible derived classes\n\n : Extensible(new Private(...))\n\nby making Extensible accept a template argument pack that can be\nforwarded to the construction of d_.\n\nI just wonder, I'm not proposing to try it :)\n\n> +\n> +/**\n> + * \\var Extensible::d_\n> + * \\brief Pointer to the private data instance\n> + */\n> +\n> +/**\n> + * \\class Extensible::Private\n> + * \\brief Base class for private data managed through a d-pointer\n> + */\n> +\n> +/**\n> + * \\brief Construct an instance of an Extensible class private data\n> + * \\param[in] o Pointer to the public class object\n> + */\n> +Extensible::Private::Private(Extensible *o)\n> +\t: o_(o)\n> +{\n> +}\n> +\n> +Extensible::Private::~Private()\n\nWhy doesn't doxygen complain ?\n\nThanks\n j\n\n> +{\n> +}\n> +\n> +/**\n> + * \\var Extensible::Private::o_\n> + * \\brief Pointer to the public class object\n> + */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 47ddb4014a61..b9f6457433f9 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -17,6 +17,7 @@ libcamera_sources = files([\n> 'event_dispatcher.cpp',\n> 'event_dispatcher_poll.cpp',\n> 'event_notifier.cpp',\n> + 'extensible.cpp',\n> 'file.cpp',\n> 'file_descriptor.cpp',\n> 'flags.cpp',\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2B456BDB13\n\tfor ;\n\tWed, 21 Oct 2020 10:04:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C165B61D98;\n\tWed, 21 Oct 2020 12:04:15 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E81BF60353\n\tfor ;\n\tWed, 21 Oct 2020 12:04:14 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 1ECF41BF208;\n\tWed, 21 Oct 2020 10:04:13 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 21 Oct 2020 12:04:12 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201021100412.2c572pwyw4xntsoi@uno.localdomain>","References":"<20201020014005.12783-1-laurent.pinchart@ideasonboard.com>\n\t<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13380,"web_url":"https://patchwork.libcamera.org/comment/13380/","msgid":"<20201021225329.GQ3942@pendragon.ideasonboard.com>","date":"2020-10-21T22:53:29","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:\n> On Tue, Oct 20, 2020 at 04:40:02AM +0300, Laurent Pinchart wrote:\n> > The d-pointer design patterns helps creating public classes that can be\n> > extended without breaking their ABI. To facilitate usage of the pattern\n> > in libcamera, create a base Extensible class with associated macros.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > Changes since v1:\n> >\n> > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros\n> > - Extend documentation\n> > - Fix typos\n> > ---\n> > include/libcamera/extensible.h | 86 +++++++++++++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/extensible.cpp | 134 +++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 222 insertions(+)\n> > create mode 100644 include/libcamera/extensible.h\n> > create mode 100644 src/libcamera/extensible.cpp\n> >\n> > diff --git a/include/libcamera/extensible.h b/include/libcamera/extensible.h\n> > new file mode 100644\n> > index 000000000000..ea8808ad3e3c\n> > --- /dev/null\n> > +++ b/include/libcamera/extensible.h\n> > @@ -0,0 +1,86 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * extensible.h - Utilities to create extensible public classes with stable ABIs\n> > + */\n> > +#ifndef __LIBCAMERA_EXTENSIBLE_H__\n> > +#define __LIBCAMERA_EXTENSIBLE_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +#ifndef __DOXYGEN__\n> > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\t\t\t\t\\\n> > +public:\t\t\t\t\t\t\t\t\t\\\n> > +\tclass Private;\t\t\t\t\t\t\t\\\n> > +\tfriend class Private;\n> > +\n> > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> > +\tfriend class klass;\n> \n> I am missing why this is needed.\n> It is meant to be used in klass::Private definition, but being Private\n> in the klass:: namespace, it shouldn't be necessary, right ?\n\nIt's meant to be used in the private class, yes. The macro name means\n\"declare the public class [for this private class]\", hence usage of\nLIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If\nthat's considered confusing, I'm fine switching the names, so that the\nmacro would mean \"declare this class as the public class\".\n\n> (In facts, I removed it from Camera::Private and\n> CameraManager::Private from patches 4/5 and 5/5 and the compiler is\n> still happy.\n\nThat's because there's currently no private class that needs to accesses\nfields from its public counterpart. I expect we'll need that later,\nbased on analysis of the d-pointer pattern implementation in Qt.\n\n> > +\n> > +#define LIBCAMERA_D_PTR(klass)\t\t\t\t\t\t\\\n> > +\t_d();\n> \n> Just and drop klass from arguments ?\n> Reading the documentation the first thing I wondered is \"why do I need\n> to pass in the public class name to get a pointer to a private\" ?\n\nDropping klass:: I can certainly do, but I think I'd prefer keeping\nLIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments\nthey take.\n\n*However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()\nas follows:\n\n#define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n\tfriend class klass;\t\t\t\t\t\t\\\n\tusing Public = klass;\n\nthen we could use _d() here, and drop the klass argument to both\nLIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?\n\nI also had a look at how we could implement an outer_type_of that\nwould resolve to U when T is U::Private. This would (I thinkg) allow\nwriting outer_type_of. I haven't been able to find a\ngood way to do so.\n\n> > +\n> > +#define LIBCAMERA_O_PTR(klass)\t\t\t\t\t\t\\\n> > +\t_o();\n> \n> I would rather provide two methods and let users call them without\n> bothering about the 'd' or 'o' name. If a class wants to call the\n> pointer to its private 'daffyduck' I don't see why we should force it\n> to be named 'd'.\n> \n> In public class:\n> \n> Private *const abcd = _private();\n\nNote that this would need to be written _private().\n\n> \n> In private class:\n> ClassName *const asdsad = _public();\n> \n> without introducing macros that has to be followed and really just\n> wrap one method call.\n\nThe variable name isn't constrained by this patch, unlike v1 that hid\nthe variable declaration in the macro. I however want to keep the names\nconsistent, as that increases readability of the code base. Anyone\nfamiliar with libcamera should be able to immediately understand what d\nand o are (d comes from the d-pointer design pattern and o stands for\nobject, but I'm fine discussing different names, especially for o),\nhence patch 1/5 in this series to enforce that rule.\n\nAs for calling functions directly instead of using macros, I think the\nmacros make it more readable by hiding the implementation details.\n\n> > +\n> > +#else\n> > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\n> > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\n> > +#define LIBCAMERA_D_PTR(klass)\n> > +#define LIBCAMERA_O_PTR(klass)\n> > +#endif\n> \n> I wlso onder if we want macros, if LIBCAMERA_ is needed in the names\n> (and I won't question the decision to call 'D' pointer a\n> pointer to the internal Private class. I understand 'O' as it might\n> recall 'Outer', but 'D' ?)\n\n'd' comes from the name of the design pattern, called d-pointer. It's\nalso called pimpl (for pointer to implementation), but that's a horrible\nname :-) As for 'o' (which I have meant as meaning object, but outer\nseems better), I initially went for 'p', but that could be both public\nor private, which isn't very nice. We could also have slightly longer\nnames if desired.\n\n> Macro names with PRIVATE and PUBLIC are more expressive imo, but it\n> might be just a matter of getting used to it.\n\nThe issue is that macros are not part of a namespace, so we need to make\nsure they won't conflict with any third-party code we could use (or that\ncould be using us, as they're in a public header, but they could\npossibly be moved to an internal header).\n\n> > +\n> > +class Extensible\n> > +{\n> > +public:\n> > +\tclass Private\n> > +\t{\n> > +\tpublic:\n> > +\t\tPrivate(Extensible *o);\n> > +\t\tvirtual ~Private();\n> > +\n> > +#ifndef __DOXYGEN__\n> > +\t\ttemplate\n> > +\t\tconst T *_o() const\n> > +\t\t{\n> > +\t\t\treturn static_cast(o_);\n> > +\t\t}\n> > +\n> > +\t\ttemplate\n> > +\t\tT *_o()\n> > +\t\t{\n> > +\t\t\treturn static_cast(o_);\n> > +\t\t}\n> > +#endif\n> > +\n> > +\tprivate:\n> > +\t\tExtensible * const o_;\n> > +\t};\n> > +\n> > +\tExtensible(Private *d);\n> > +\n> > +protected:\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate\n> > +\tconst T *_d() const\n> > +\t{\n> > +\t\treturn static_cast(d_.get());\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tT *_d()\n> > +\t{\n> > +\t\treturn static_cast(d_.get());\n> > +\t}\n> > +#endif\n> > +\n> > +private:\n> > +\tconst std::unique_ptr d_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_EXTENSIBLE_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 83bc46729314..15e6b43c9585 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -8,6 +8,7 @@ libcamera_public_headers = files([\n> > 'controls.h',\n> > 'event_dispatcher.h',\n> > 'event_notifier.h',\n> > + 'extensible.h',\n> > 'file_descriptor.h',\n> > 'flags.h',\n> > 'framebuffer_allocator.h',\n> > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp\n> > new file mode 100644\n> > index 000000000000..1dcb0bf1b12f\n> > --- /dev/null\n> > +++ b/src/libcamera/extensible.cpp\n> > @@ -0,0 +1,134 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * extensible.cpp - Utilities to create extensible public classes with stable ABIs\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file extensible.h\n> > + * \\brief Utilities to create extensible public classes with stable ABIs\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\def LIBCAMERA_DECLARE_PRIVATE\n> > + * \\brief Declare private data for a public class\n> > + * \\param klass The public class name\n> > + *\n> > + * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to\n> > + * make a class manage its private data through a d-pointer. It shall be used at\n> > + * the very top of the class definition, with the public class name passed as\n> > + * the \\a klass parameter.\n> > + */\n> > +\n> > +/**\n> > + * \\def LIBCAMERA_DECLARE_PUBLIC\n> > + * \\brief Declare public data for a private class\n> > + * \\param klass The public class name\n> > + *\n> > + * The LIBCAMERA_DECLARE_PUBLIC() macro is the counterpart of\n> > + * LIBCAMERA_DECLARE_PRIVATE() to be used in the private data class. It shall be\n> > + * used at the very top of the private class definition, with the public class\n> > + * name passed as the \\a klass parameter.\n> > + */\n> > +\n> > +/**\n> > + * \\def LIBCAMERA_D_PTR(klass)\n> > + * \\brief Retrieve the private data pointer\n> > + * \\param[in] klass The public class name\n> > + *\n> > + * This macro can be used in any member function of a class that inherits,\n> > + * directly or indirectly, from the Extensible class, to create a local\n> > + * variable named 'd' that points to the class' private data instance.\n> > + */\n> > +\n> > +/**\n> > + * \\def LIBCAMERA_O_PTR(klass)\n> > + * \\brief Retrieve the public instance corresponding to the private data\n> > + * \\param[in] klass The public class name\n> > + *\n> > + * This macro is the counterpart of LIBCAMERA_D_PTR() for private data classes.\n> > + * It can be used in any member function of the private data class to create a\n> > + * local variable named 'o' that points to the public class instance\n> > + * corresponding to the private data.\n> \n> The two macros do not \"create a local variable named ['o'|'d'] anymore\n\nOops. I'll fix this (once we come to an agreement on the above\ndiscussion).\n\n> > + */\n> > +\n> > +/**\n> > + * \\class Extensible\n> > + * \\brief Base class to manage private data through a d-pointer\n> > + *\n> > + * The Extensible class provides a base class to implement the\n> > + * d-pointer design pattern (also\n> > + * known as opaque pointer\n> > + * or pImpl idiom).\n> > + * It helps creating public classes that can be extended without breaking their\n> > + * ABI. Such classes store their private data in a separate private data object,\n> > + * referenced by a pointer in the public class (hence the name d-pointer).\n> > + *\n> > + * Classes that follow this design pattern are referred herein as extensible\n> > + * classes. To be extensible, a class PublicClass shall:\n> > + *\n> > + * - inherit from the Extensible class or from another extensible class\n> > + * - invoke the LIBCAMERA_DECLARE_PRIVATE() macro at the very top of the class\n> > + * definition\n> > + * - define a private data class named PublicClass::Private that inherits from\n> > + * the Private data class of the base class\n> > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private\n> > + * data class definition\n> > + * - pass a pointer to a newly allocated Private data object to the constructor\n> > + * of the base class\n> > + *\n> > + * Additionally, if the PublicClass is not final, it shall expose one or more\n> > + * constructors that takes a pointer to a Private data instance, to be used by\n> > + * derived classes.\n> > + *\n> > + * The Private class is fully opaque to users of the libcamera public API.\n> > + * Internally, it can be kept private to the implementation of PublicClass, or\n> > + * be exposed to other classes. In the latter case, the members of the Private\n> > + * class need to be qualified with appropriate access specifiers. The\n> > + * PublicClass and Private classes always have full access to each other's\n> > + * protected and private members.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an instance of an Extensible class\n> > + * \\param[in] d Pointer to the private data instance\n> > + */\n> > +Extensible::Extensible(Extensible::Private *d)\n> > +\t: d_(d)\n> > +{\n> > +}\n> \n> I wonder if we could avoid hainvg in the extensible derived classes\n> \n> : Extensible(new Private(...))\n> \n> by making Extensible accept a template argument pack that can be\n> forwarded to the construction of d_.\n> \n> I just wonder, I'm not proposing to try it :)\n\nI'm not sure to follow you, do you have an example ?\n\n> > +\n> > +/**\n> > + * \\var Extensible::d_\n> > + * \\brief Pointer to the private data instance\n> > + */\n> > +\n> > +/**\n> > + * \\class Extensible::Private\n> > + * \\brief Base class for private data managed through a d-pointer\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct an instance of an Extensible class private data\n> > + * \\param[in] o Pointer to the public class object\n> > + */\n> > +Extensible::Private::Private(Extensible *o)\n> > +\t: o_(o)\n> > +{\n> > +}\n> > +\n> > +Extensible::Private::~Private()\n> \n> Why doesn't doxygen complain ?\n\nBecause it contains this:\n\nEXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n libcamera::BoundMethodBase \\\n libcamera::BoundMethodMember \\\n libcamera::BoundMethodPack \\\n libcamera::BoundMethodPackBase \\\n libcamera::BoundMethodStatic \\\n libcamera::SignalBase \\\n libcamera::*::Private \\\n *::details::* \\\n std::*\n\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\var Extensible::Private::o_\n> > + * \\brief Pointer to the public class object\n> > + */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 47ddb4014a61..b9f6457433f9 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -17,6 +17,7 @@ libcamera_sources = files([\n> > 'event_dispatcher.cpp',\n> > 'event_dispatcher_poll.cpp',\n> > 'event_notifier.cpp',\n> > + 'extensible.cpp',\n> > 'file.cpp',\n> > 'file_descriptor.cpp',\n> > 'flags.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8A18CBDB13\n\tfor ;\n\tWed, 21 Oct 2020 22:54:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BE30610FD;\n\tThu, 22 Oct 2020 00:54:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A33AD603B0\n\tfor ;\n\tThu, 22 Oct 2020 00:54:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E14C052F;\n\tThu, 22 Oct 2020 00:54:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"C1wMxQs9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603320855;\n\tbh=gNKHrBA54Hfzau81DFFNfEjitPE7uRL/S7CYpjyEoWw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C1wMxQs9z9H9CuHP4ncJ0lroxvjl9XBVytv99DqXvZajpb5tDoYn6g9eiy0jl2fNX\n\tq8RgQ9TRcLak6OE/maUe1GJYT0ZXuE37255+qtH73/yK5zwqvboj2yGqhT0lb4OVmo\n\tnBXTRoVm2bTz4zUZy9gv7h4jgmyUEQ1LgA83iW9s=","Date":"Thu, 22 Oct 2020 01:53:29 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201021225329.GQ3942@pendragon.ideasonboard.com>","References":"<20201020014005.12783-1-laurent.pinchart@ideasonboard.com>\n\t<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>\n\t<20201021100412.2c572pwyw4xntsoi@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201021100412.2c572pwyw4xntsoi@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13514,"web_url":"https://patchwork.libcamera.org/comment/13514/","msgid":"<20201027121106.hphsgsoioqfmakpy@uno.localdomain>","date":"2020-10-27T12:11:06","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 22, 2020 at 01:53:29AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:\n> > On Tue, Oct 20, 2020 at 04:40:02AM +0300, Laurent Pinchart wrote:\n> > > The d-pointer design patterns helps creating public classes that can be\n> > > extended without breaking their ABI. To facilitate usage of the pattern\n> > > in libcamera, create a base Extensible class with associated macros.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Niklas Söderlund \n> > > ---\n> > > Changes since v1:\n> > >\n> > > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros\n> > > - Extend documentation\n> > > - Fix typos\n> > > ---\n> > > include/libcamera/extensible.h | 86 +++++++++++++++++++++\n> > > include/libcamera/meson.build | 1 +\n> > > src/libcamera/extensible.cpp | 134 +++++++++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 4 files changed, 222 insertions(+)\n> > > create mode 100644 include/libcamera/extensible.h\n> > > create mode 100644 src/libcamera/extensible.cpp\n> > >\n> > > diff --git a/include/libcamera/extensible.h b/include/libcamera/extensible.h\n> > > new file mode 100644\n> > > index 000000000000..ea8808ad3e3c\n> > > --- /dev/null\n> > > +++ b/include/libcamera/extensible.h\n> > > @@ -0,0 +1,86 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * extensible.h - Utilities to create extensible public classes with stable ABIs\n> > > + */\n> > > +#ifndef __LIBCAMERA_EXTENSIBLE_H__\n> > > +#define __LIBCAMERA_EXTENSIBLE_H__\n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\t\t\t\t\\\n> > > +public:\t\t\t\t\t\t\t\t\t\\\n> > > +\tclass Private;\t\t\t\t\t\t\t\\\n> > > +\tfriend class Private;\n> > > +\n> > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> > > +\tfriend class klass;\n> >\n> > I am missing why this is needed.\n> > It is meant to be used in klass::Private definition, but being Private\n> > in the klass:: namespace, it shouldn't be necessary, right ?\n>\n> It's meant to be used in the private class, yes. The macro name means\n> \"declare the public class [for this private class]\", hence usage of\n> LIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If\n> that's considered confusing, I'm fine switching the names, so that the\n> macro would mean \"declare this class as the public class\".\n>\n\nI meant I was not sure why the 'friend' declaration is required, as\nthe Private class is declared inside the Public one and thus should be\naccessible\n\n> > (In facts, I removed it from Camera::Private and\n> > CameraManager::Private from patches 4/5 and 5/5 and the compiler is\n> > still happy.\n>\n> That's because there's currently no private class that needs to accesses\n> fields from its public counterpart. I expect we'll need that later,\n> based on analysis of the d-pointer pattern implementation in Qt.\n>\n\nI see.\n\nMy doubt on Private being declared inside Public thus not requiring\nthe \"friend class klass\" has been clarified by the compiler\ncomplaining because of\n\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -36,8 +36,6 @@ LOG_DEFINE_CATEGORY(Camera)\n\n class CameraManager::Private : public Extensible::Private, public Thread\n {\n- LIBCAMERA_DECLARE_PUBLIC(CameraManager)\n-\n public:\n Private(CameraManager *cm);\n\n@@ -72,6 +70,8 @@ private:\n\n IPAManager ipaManager_;\n ProcessManager processManager_;\n+\n+ int a;\n };\n\n CameraManager::Private::Private(CameraManager *cm)\n@@ -296,6 +296,9 @@ int CameraManager::start()\n {\n Private *const d = LIBCAMERA_D_PTR(CameraManager);\n\n+ if (d->a)\n+ return 0;\n+\n LOG(Camera, Info) << \"libcamera \" << version_;\n\n int ret = d->start();\n\n> > > +\n> > > +#define LIBCAMERA_D_PTR(klass)\t\t\t\t\t\t\\\n> > > +\t_d();\n> >\n> > Just and drop klass from arguments ?\n> > Reading the documentation the first thing I wondered is \"why do I need\n> > to pass in the public class name to get a pointer to a private\" ?\n>\n> Dropping klass:: I can certainly do, but I think I'd prefer keeping\n> LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments\n> they take.\n>\n> *However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()\n> as follows:\n>\n> #define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> \tfriend class klass;\t\t\t\t\t\t\\\n> \tusing Public = klass;\n>\n> then we could use _d() here, and drop the klass argument to both\n> LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?\n\nI like it better, but if you prefer symetrical macros, that's fine too\n\n>\n> I also had a look at how we could implement an outer_type_of that\n> would resolve to U when T is U::Private. This would (I thinkg) allow\n> writing outer_type_of. I haven't been able to find a\n> good way to do so.\n>\n\nNot following you down this rabbit hole, I'm sorry :)\n\n> > > +\n> > > +#define LIBCAMERA_O_PTR(klass)\t\t\t\t\t\t\\\n> > > +\t_o();\n> >\n> > I would rather provide two methods and let users call them without\n> > bothering about the 'd' or 'o' name. If a class wants to call the\n> > pointer to its private 'daffyduck' I don't see why we should force it\n> > to be named 'd'.\n> >\n> > In public class:\n> >\n> > Private *const abcd = _private();\n>\n> Note that this would need to be written _private().\n>\n> >\n> > In private class:\n> > ClassName *const asdsad = _public();\n> >\n> > without introducing macros that has to be followed and really just\n> > wrap one method call.\n>\n> The variable name isn't constrained by this patch, unlike v1 that hid\n> the variable declaration in the macro. I however want to keep the names\n> consistent, as that increases readability of the code base. Anyone\n> familiar with libcamera should be able to immediately understand what d\n> and o are (d comes from the d-pointer design pattern and o stands for\n> object, but I'm fine discussing different names, especially for o),\n> hence patch 1/5 in this series to enforce that rule.\n\nYeah, I meant not reporting an error in checkstyle.\n\n>\n> As for calling functions directly instead of using macros, I think the\n> macros make it more readable by hiding the implementation details.\n>\n\nOk, so, and sorry for backtracking, if you want 'd' and 'o' to be\nforced and get it as recognizible libcamera construct, macros are fine\ntoo\n\n> > > +\n> > > +#else\n> > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\n> > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\n> > > +#define LIBCAMERA_D_PTR(klass)\n> > > +#define LIBCAMERA_O_PTR(klass)\n> > > +#endif\n> >\n> > I wlso onder if we want macros, if LIBCAMERA_ is needed in the names\n> > (and I won't question the decision to call 'D' pointer a\n> > pointer to the internal Private class. I understand 'O' as it might\n> > recall 'Outer', but 'D' ?)\n>\n> 'd' comes from the name of the design pattern, called d-pointer. It's\n> also called pimpl (for pointer to implementation), but that's a horrible\n> name :-) As for 'o' (which I have meant as meaning object, but outer\n> seems better), I initially went for 'p', but that could be both public\n> or private, which isn't very nice. We could also have slightly longer\n> names if desired.\n>\n> > Macro names with PRIVATE and PUBLIC are more expressive imo, but it\n> > might be just a matter of getting used to it.\n>\n> The issue is that macros are not part of a namespace, so we need to make\n> sure they won't conflict with any third-party code we could use (or that\n> could be using us, as they're in a public header, but they could\n> possibly be moved to an internal header).\n\nI'm fine keeping LIBCAMERA_ then\n\n>\n> > > +\n> > > +class Extensible\n> > > +{\n> > > +public:\n> > > +\tclass Private\n> > > +\t{\n> > > +\tpublic:\n> > > +\t\tPrivate(Extensible *o);\n> > > +\t\tvirtual ~Private();\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +\t\ttemplate\n> > > +\t\tconst T *_o() const\n> > > +\t\t{\n> > > +\t\t\treturn static_cast(o_);\n> > > +\t\t}\n> > > +\n> > > +\t\ttemplate\n> > > +\t\tT *_o()\n> > > +\t\t{\n> > > +\t\t\treturn static_cast(o_);\n> > > +\t\t}\n> > > +#endif\n> > > +\n> > > +\tprivate:\n> > > +\t\tExtensible * const o_;\n> > > +\t};\n> > > +\n> > > +\tExtensible(Private *d);\n> > > +\n> > > +protected:\n> > > +#ifndef __DOXYGEN__\n> > > +\ttemplate\n> > > +\tconst T *_d() const\n> > > +\t{\n> > > +\t\treturn static_cast(d_.get());\n> > > +\t}\n> > > +\n> > > +\ttemplate\n> > > +\tT *_d()\n> > > +\t{\n> > > +\t\treturn static_cast(d_.get());\n> > > +\t}\n> > > +#endif\n> > > +\n> > > +private:\n> > > +\tconst std::unique_ptr d_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif /* __LIBCAMERA_EXTENSIBLE_H__ */\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 83bc46729314..15e6b43c9585 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -8,6 +8,7 @@ libcamera_public_headers = files([\n> > > 'controls.h',\n> > > 'event_dispatcher.h',\n> > > 'event_notifier.h',\n> > > + 'extensible.h',\n> > > 'file_descriptor.h',\n> > > 'flags.h',\n> > > 'framebuffer_allocator.h',\n> > > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp\n> > > new file mode 100644\n> > > index 000000000000..1dcb0bf1b12f\n> > > --- /dev/null\n> > > +++ b/src/libcamera/extensible.cpp\n> > > @@ -0,0 +1,134 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * extensible.cpp - Utilities to create extensible public classes with stable ABIs\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file extensible.h\n> > > + * \\brief Utilities to create extensible public classes with stable ABIs\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\def LIBCAMERA_DECLARE_PRIVATE\n> > > + * \\brief Declare private data for a public class\n> > > + * \\param klass The public class name\n> > > + *\n> > > + * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to\n> > > + * make a class manage its private data through a d-pointer. It shall be used at\n> > > + * the very top of the class definition, with the public class name passed as\n> > > + * the \\a klass parameter.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\def LIBCAMERA_DECLARE_PUBLIC\n> > > + * \\brief Declare public data for a private class\n> > > + * \\param klass The public class name\n> > > + *\n> > > + * The LIBCAMERA_DECLARE_PUBLIC() macro is the counterpart of\n> > > + * LIBCAMERA_DECLARE_PRIVATE() to be used in the private data class. It shall be\n> > > + * used at the very top of the private class definition, with the public class\n> > > + * name passed as the \\a klass parameter.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\def LIBCAMERA_D_PTR(klass)\n> > > + * \\brief Retrieve the private data pointer\n> > > + * \\param[in] klass The public class name\n> > > + *\n> > > + * This macro can be used in any member function of a class that inherits,\n> > > + * directly or indirectly, from the Extensible class, to create a local\n> > > + * variable named 'd' that points to the class' private data instance.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\def LIBCAMERA_O_PTR(klass)\n> > > + * \\brief Retrieve the public instance corresponding to the private data\n> > > + * \\param[in] klass The public class name\n> > > + *\n> > > + * This macro is the counterpart of LIBCAMERA_D_PTR() for private data classes.\n> > > + * It can be used in any member function of the private data class to create a\n> > > + * local variable named 'o' that points to the public class instance\n> > > + * corresponding to the private data.\n> >\n> > The two macros do not \"create a local variable named ['o'|'d'] anymore\n>\n> Oops. I'll fix this (once we come to an agreement on the above\n> discussion).\n>\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class Extensible\n> > > + * \\brief Base class to manage private data through a d-pointer\n> > > + *\n> > > + * The Extensible class provides a base class to implement the\n> > > + * d-pointer design pattern (also\n> > > + * known as opaque pointer\n> > > + * or pImpl idiom).\n> > > + * It helps creating public classes that can be extended without breaking their\n> > > + * ABI. Such classes store their private data in a separate private data object,\n> > > + * referenced by a pointer in the public class (hence the name d-pointer).\n> > > + *\n> > > + * Classes that follow this design pattern are referred herein as extensible\n> > > + * classes. To be extensible, a class PublicClass shall:\n> > > + *\n> > > + * - inherit from the Extensible class or from another extensible class\n> > > + * - invoke the LIBCAMERA_DECLARE_PRIVATE() macro at the very top of the class\n> > > + * definition\n> > > + * - define a private data class named PublicClass::Private that inherits from\n> > > + * the Private data class of the base class\n> > > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private\n> > > + * data class definition\n> > > + * - pass a pointer to a newly allocated Private data object to the constructor\n> > > + * of the base class\n> > > + *\n> > > + * Additionally, if the PublicClass is not final, it shall expose one or more\n> > > + * constructors that takes a pointer to a Private data instance, to be used by\n> > > + * derived classes.\n> > > + *\n> > > + * The Private class is fully opaque to users of the libcamera public API.\n> > > + * Internally, it can be kept private to the implementation of PublicClass, or\n> > > + * be exposed to other classes. In the latter case, the members of the Private\n> > > + * class need to be qualified with appropriate access specifiers. The\n> > > + * PublicClass and Private classes always have full access to each other's\n> > > + * protected and private members.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an instance of an Extensible class\n> > > + * \\param[in] d Pointer to the private data instance\n> > > + */\n> > > +Extensible::Extensible(Extensible::Private *d)\n> > > +\t: d_(d)\n> > > +{\n> > > +}\n> >\n> > I wonder if we could avoid hainvg in the extensible derived classes\n> >\n> > : Extensible(new Private(...))\n> >\n> > by making Extensible accept a template argument pack that can be\n> > forwarded to the construction of d_.\n> >\n> > I just wonder, I'm not proposing to try it :)\n>\n> I'm not sure to follow you, do you have an example ?\n>\n\nDon't worry, I was just thinking out loud. Please ignore this.\n\n> > > +\n> > > +/**\n> > > + * \\var Extensible::d_\n> > > + * \\brief Pointer to the private data instance\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class Extensible::Private\n> > > + * \\brief Base class for private data managed through a d-pointer\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct an instance of an Extensible class private data\n> > > + * \\param[in] o Pointer to the public class object\n> > > + */\n> > > +Extensible::Private::Private(Extensible *o)\n> > > +\t: o_(o)\n> > > +{\n> > > +}\n> > > +\n> > > +Extensible::Private::~Private()\n> >\n> > Why doesn't doxygen complain ?\n>\n> Because it contains this:\n>\n> EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> libcamera::BoundMethodBase \\\n> libcamera::BoundMethodMember \\\n> libcamera::BoundMethodPack \\\n> libcamera::BoundMethodPackBase \\\n> libcamera::BoundMethodStatic \\\n> libcamera::SignalBase \\\n> libcamera::*::Private \\\n> *::details::* \\\n> std::*\n\nAck.\n\nOverall I think I've pestered you enough. If you want to enforceusage\nof 'd' and 'o' having macros to either declare the variable or just shorten\nthe access is fine. I still lean towards letting classes open code\naccess to private and public classes, without enforcing naming, but\neverything is fine.\n\nFor next version, with minors fixed:\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var Extensible::Private::o_\n> > > + * \\brief Pointer to the public class object\n> > > + */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 47ddb4014a61..b9f6457433f9 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -17,6 +17,7 @@ libcamera_sources = files([\n> > > 'event_dispatcher.cpp',\n> > > 'event_dispatcher_poll.cpp',\n> > > 'event_notifier.cpp',\n> > > + 'extensible.cpp',\n> > > 'file.cpp',\n> > > 'file_descriptor.cpp',\n> > > 'flags.cpp',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 875A7C3B5C\n\tfor ;\n\tTue, 27 Oct 2020 12:11:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 242DF620D5;\n\tTue, 27 Oct 2020 13:11:10 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 480FE62078\n\tfor ;\n\tTue, 27 Oct 2020 13:11:09 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A222DE000C;\n\tTue, 27 Oct 2020 12:11:08 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 27 Oct 2020 13:11:06 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201027121106.hphsgsoioqfmakpy@uno.localdomain>","References":"<20201020014005.12783-1-laurent.pinchart@ideasonboard.com>\n\t<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>\n\t<20201021100412.2c572pwyw4xntsoi@uno.localdomain>\n\t<20201021225329.GQ3942@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201021225329.GQ3942@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13515,"web_url":"https://patchwork.libcamera.org/comment/13515/","msgid":"<20201027121915.GA24666@pendragon.ideasonboard.com>","date":"2020-10-27T12:19:15","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Oct 27, 2020 at 01:11:06PM +0100, Jacopo Mondi wrote:\n> On Thu, Oct 22, 2020 at 01:53:29AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 21, 2020 at 12:04:12PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Oct 20, 2020 at 04:40:02AM +0300, Laurent Pinchart wrote:\n> > > > The d-pointer design patterns helps creating public classes that can be\n> > > > extended without breaking their ABI. To facilitate usage of the pattern\n> > > > in libcamera, create a base Extensible class with associated macros.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > Reviewed-by: Niklas Söderlund \n> > > > ---\n> > > > Changes since v1:\n> > > >\n> > > > - Don't hide variable declarations in LIBCAMERA_[DO]_PTR macros\n> > > > - Extend documentation\n> > > > - Fix typos\n> > > > ---\n> > > > include/libcamera/extensible.h | 86 +++++++++++++++++++++\n> > > > include/libcamera/meson.build | 1 +\n> > > > src/libcamera/extensible.cpp | 134 +++++++++++++++++++++++++++++++++\n> > > > src/libcamera/meson.build | 1 +\n> > > > 4 files changed, 222 insertions(+)\n> > > > create mode 100644 include/libcamera/extensible.h\n> > > > create mode 100644 src/libcamera/extensible.cpp\n> > > >\n> > > > diff --git a/include/libcamera/extensible.h b/include/libcamera/extensible.h\n> > > > new file mode 100644\n> > > > index 000000000000..ea8808ad3e3c\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/extensible.h\n> > > > @@ -0,0 +1,86 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * extensible.h - Utilities to create extensible public classes with stable ABIs\n> > > > + */\n> > > > +#ifndef __LIBCAMERA_EXTENSIBLE_H__\n> > > > +#define __LIBCAMERA_EXTENSIBLE_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +#ifndef __DOXYGEN__\n> > > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\t\t\t\t\\\n> > > > +public:\t\t\t\t\t\t\t\t\t\\\n> > > > +\tclass Private;\t\t\t\t\t\t\t\\\n> > > > +\tfriend class Private;\n> > > > +\n> > > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> > > > +\tfriend class klass;\n> > >\n> > > I am missing why this is needed.\n> > > It is meant to be used in klass::Private definition, but being Private\n> > > in the klass:: namespace, it shouldn't be necessary, right ?\n> >\n> > It's meant to be used in the private class, yes. The macro name means\n> > \"declare the public class [for this private class]\", hence usage of\n> > LIBCAMERA_DECLARE_PUBLIC() in the private class, and vice versa. If\n> > that's considered confusing, I'm fine switching the names, so that the\n> > macro would mean \"declare this class as the public class\".\n> \n> I meant I was not sure why the 'friend' declaration is required, as\n> the Private class is declared inside the Public one and thus should be\n> accessible\n\nclass Public\n{ \npublic:\n\tPublic();\n\nprivate:\n\tclass Private\n\t{\n\tpublic:\n\t\tPrivate();\n\tprivate:\n\t\tint data_;\n\t};\n\n\tPrivate *private_;\n};\n\nPublic::Public()\n{\n\tprivate_ = new Private();\n\tprivate_->data_ = 0;\n}\n\nprivate.cpp: In constructor ‘Public::Public()’:\nprivate.cpp:21:12: error: ‘int Public::Private::data_’ is private within this context\n private_->data_ = 0;\n ^~~~~\nprivate.cpp:12:7: note: declared private here\n int data_;\n ^~~~~\n\n> > > (In facts, I removed it from Camera::Private and\n> > > CameraManager::Private from patches 4/5 and 5/5 and the compiler is\n> > > still happy.\n> >\n> > That's because there's currently no private class that needs to accesses\n> > fields from its public counterpart. I expect we'll need that later,\n> > based on analysis of the d-pointer pattern implementation in Qt.\n> \n> I see.\n> \n> My doubt on Private being declared inside Public thus not requiring\n> the \"friend class klass\" has been clarified by the compiler\n> complaining because of\n> \n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -36,8 +36,6 @@ LOG_DEFINE_CATEGORY(Camera)\n> \n> class CameraManager::Private : public Extensible::Private, public Thread\n> {\n> - LIBCAMERA_DECLARE_PUBLIC(CameraManager)\n> -\n> public:\n> Private(CameraManager *cm);\n> \n> @@ -72,6 +70,8 @@ private:\n> \n> IPAManager ipaManager_;\n> ProcessManager processManager_;\n> +\n> + int a;\n> };\n> \n> CameraManager::Private::Private(CameraManager *cm)\n> @@ -296,6 +296,9 @@ int CameraManager::start()\n> {\n> Private *const d = LIBCAMERA_D_PTR(CameraManager);\n> \n> + if (d->a)\n> + return 0;\n> +\n> LOG(Camera, Info) << \"libcamera \" << version_;\n> \n> int ret = d->start();\n\nRight, we're tested the same thing :-)\n\n> > > > +\n> > > > +#define LIBCAMERA_D_PTR(klass)\t\t\t\t\t\t\\\n> > > > +\t_d();\n> > >\n> > > Just and drop klass from arguments ?\n> > > Reading the documentation the first thing I wondered is \"why do I need\n> > > to pass in the public class name to get a pointer to a private\" ?\n> >\n> > Dropping klass:: I can certainly do, but I think I'd prefer keeping\n> > LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR() symmetrical in the arguments\n> > they take.\n> >\n> > *However*, this could be solved by extending LIBCAMERA_DECLARE_PUBLIC()\n> > as follows:\n> >\n> > #define LIBCAMERA_DECLARE_PUBLIC(klass)\t\t\t\t\t\\\n> > \tfriend class klass;\t\t\t\t\t\t\\\n> > \tusing Public = klass;\n> >\n> > then we could use _d() here, and drop the klass argument to both\n> > LIBCAMERA_D_PTR() and LIBCAMERA_O_PTR(). Would you like that better ?\n> \n> I like it better, but if you prefer symetrical macros, that's fine too\n\nI'd prefer LIBCAMERA_DECLARE_PUBLIC() and LIBCAMERA_DECLARE_PRIVATE() to\nbe symmetrical indeed, but we can at least drop the klass argument to\nLIBCAMERA_D_PTR() and LIBCAMERA_O_PTR().\n\n> > I also had a look at how we could implement an outer_type_of that\n> > would resolve to U when T is U::Private. This would (I thinkg) allow\n> > writing outer_type_of. I haven't been able to find a\n> > good way to do so.\n> \n> Not following you down this rabbit hole, I'm sorry :)\n\n:-)\n\n> > > > +\n> > > > +#define LIBCAMERA_O_PTR(klass)\t\t\t\t\t\t\\\n> > > > +\t_o();\n> > >\n> > > I would rather provide two methods and let users call them without\n> > > bothering about the 'd' or 'o' name. If a class wants to call the\n> > > pointer to its private 'daffyduck' I don't see why we should force it\n> > > to be named 'd'.\n> > >\n> > > In public class:\n> > >\n> > > Private *const abcd = _private();\n> >\n> > Note that this would need to be written _private().\n> >\n> > >\n> > > In private class:\n> > > ClassName *const asdsad = _public();\n> > >\n> > > without introducing macros that has to be followed and really just\n> > > wrap one method call.\n> >\n> > The variable name isn't constrained by this patch, unlike v1 that hid\n> > the variable declaration in the macro. I however want to keep the names\n> > consistent, as that increases readability of the code base. Anyone\n> > familiar with libcamera should be able to immediately understand what d\n> > and o are (d comes from the d-pointer design pattern and o stands for\n> > object, but I'm fine discussing different names, especially for o),\n> > hence patch 1/5 in this series to enforce that rule.\n> \n> Yeah, I meant not reporting an error in checkstyle.\n> \n> > As for calling functions directly instead of using macros, I think the\n> > macros make it more readable by hiding the implementation details.\n> \n> Ok, so, and sorry for backtracking, if you want 'd' and 'o' to be\n> forced and get it as recognizible libcamera construct, macros are fine\n> too\n\nJust to be sure, does this mean you prefer\n\n\tPrivate * const d = LIBCAMERA_D_PTR();\n\nor\n\n\tLIBCAMERA_D_PTR();\n\nwith the d variable hidden in the macro ?\n\n> > > > +\n> > > > +#else\n> > > > +#define LIBCAMERA_DECLARE_PRIVATE(klass)\n> > > > +#define LIBCAMERA_DECLARE_PUBLIC(klass)\n> > > > +#define LIBCAMERA_D_PTR(klass)\n> > > > +#define LIBCAMERA_O_PTR(klass)\n> > > > +#endif\n> > >\n> > > I wlso onder if we want macros, if LIBCAMERA_ is needed in the names\n> > > (and I won't question the decision to call 'D' pointer a\n> > > pointer to the internal Private class. I understand 'O' as it might\n> > > recall 'Outer', but 'D' ?)\n> >\n> > 'd' comes from the name of the design pattern, called d-pointer. It's\n> > also called pimpl (for pointer to implementation), but that's a horrible\n> > name :-) As for 'o' (which I have meant as meaning object, but outer\n> > seems better), I initially went for 'p', but that could be both public\n> > or private, which isn't very nice. We could also have slightly longer\n> > names if desired.\n> >\n> > > Macro names with PRIVATE and PUBLIC are more expressive imo, but it\n> > > might be just a matter of getting used to it.\n> >\n> > The issue is that macros are not part of a namespace, so we need to make\n> > sure they won't conflict with any third-party code we could use (or that\n> > could be using us, as they're in a public header, but they could\n> > possibly be moved to an internal header).\n> \n> I'm fine keeping LIBCAMERA_ then\n> \n> > > > +\n> > > > +class Extensible\n> > > > +{\n> > > > +public:\n> > > > +\tclass Private\n> > > > +\t{\n> > > > +\tpublic:\n> > > > +\t\tPrivate(Extensible *o);\n> > > > +\t\tvirtual ~Private();\n> > > > +\n> > > > +#ifndef __DOXYGEN__\n> > > > +\t\ttemplate\n> > > > +\t\tconst T *_o() const\n> > > > +\t\t{\n> > > > +\t\t\treturn static_cast(o_);\n> > > > +\t\t}\n> > > > +\n> > > > +\t\ttemplate\n> > > > +\t\tT *_o()\n> > > > +\t\t{\n> > > > +\t\t\treturn static_cast(o_);\n> > > > +\t\t}\n> > > > +#endif\n> > > > +\n> > > > +\tprivate:\n> > > > +\t\tExtensible * const o_;\n> > > > +\t};\n> > > > +\n> > > > +\tExtensible(Private *d);\n> > > > +\n> > > > +protected:\n> > > > +#ifndef __DOXYGEN__\n> > > > +\ttemplate\n> > > > +\tconst T *_d() const\n> > > > +\t{\n> > > > +\t\treturn static_cast(d_.get());\n> > > > +\t}\n> > > > +\n> > > > +\ttemplate\n> > > > +\tT *_d()\n> > > > +\t{\n> > > > +\t\treturn static_cast(d_.get());\n> > > > +\t}\n> > > > +#endif\n> > > > +\n> > > > +private:\n> > > > +\tconst std::unique_ptr d_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > +\n> > > > +#endif /* __LIBCAMERA_EXTENSIBLE_H__ */\n> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > > index 83bc46729314..15e6b43c9585 100644\n> > > > --- a/include/libcamera/meson.build\n> > > > +++ b/include/libcamera/meson.build\n> > > > @@ -8,6 +8,7 @@ libcamera_public_headers = files([\n> > > > 'controls.h',\n> > > > 'event_dispatcher.h',\n> > > > 'event_notifier.h',\n> > > > + 'extensible.h',\n> > > > 'file_descriptor.h',\n> > > > 'flags.h',\n> > > > 'framebuffer_allocator.h',\n> > > > diff --git a/src/libcamera/extensible.cpp b/src/libcamera/extensible.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..1dcb0bf1b12f\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/extensible.cpp\n> > > > @@ -0,0 +1,134 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * extensible.cpp - Utilities to create extensible public classes with stable ABIs\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +/**\n> > > > + * \\file extensible.h\n> > > > + * \\brief Utilities to create extensible public classes with stable ABIs\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +/**\n> > > > + * \\def LIBCAMERA_DECLARE_PRIVATE\n> > > > + * \\brief Declare private data for a public class\n> > > > + * \\param klass The public class name\n> > > > + *\n> > > > + * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to\n> > > > + * make a class manage its private data through a d-pointer. It shall be used at\n> > > > + * the very top of the class definition, with the public class name passed as\n> > > > + * the \\a klass parameter.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\def LIBCAMERA_DECLARE_PUBLIC\n> > > > + * \\brief Declare public data for a private class\n> > > > + * \\param klass The public class name\n> > > > + *\n> > > > + * The LIBCAMERA_DECLARE_PUBLIC() macro is the counterpart of\n> > > > + * LIBCAMERA_DECLARE_PRIVATE() to be used in the private data class. It shall be\n> > > > + * used at the very top of the private class definition, with the public class\n> > > > + * name passed as the \\a klass parameter.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\def LIBCAMERA_D_PTR(klass)\n> > > > + * \\brief Retrieve the private data pointer\n> > > > + * \\param[in] klass The public class name\n> > > > + *\n> > > > + * This macro can be used in any member function of a class that inherits,\n> > > > + * directly or indirectly, from the Extensible class, to create a local\n> > > > + * variable named 'd' that points to the class' private data instance.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\def LIBCAMERA_O_PTR(klass)\n> > > > + * \\brief Retrieve the public instance corresponding to the private data\n> > > > + * \\param[in] klass The public class name\n> > > > + *\n> > > > + * This macro is the counterpart of LIBCAMERA_D_PTR() for private data classes.\n> > > > + * It can be used in any member function of the private data class to create a\n> > > > + * local variable named 'o' that points to the public class instance\n> > > > + * corresponding to the private data.\n> > >\n> > > The two macros do not \"create a local variable named ['o'|'d'] anymore\n> >\n> > Oops. I'll fix this (once we come to an agreement on the above\n> > discussion).\n> >\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class Extensible\n> > > > + * \\brief Base class to manage private data through a d-pointer\n> > > > + *\n> > > > + * The Extensible class provides a base class to implement the\n> > > > + * d-pointer design pattern (also\n> > > > + * known as opaque pointer\n> > > > + * or pImpl idiom).\n> > > > + * It helps creating public classes that can be extended without breaking their\n> > > > + * ABI. Such classes store their private data in a separate private data object,\n> > > > + * referenced by a pointer in the public class (hence the name d-pointer).\n> > > > + *\n> > > > + * Classes that follow this design pattern are referred herein as extensible\n> > > > + * classes. To be extensible, a class PublicClass shall:\n> > > > + *\n> > > > + * - inherit from the Extensible class or from another extensible class\n> > > > + * - invoke the LIBCAMERA_DECLARE_PRIVATE() macro at the very top of the class\n> > > > + * definition\n> > > > + * - define a private data class named PublicClass::Private that inherits from\n> > > > + * the Private data class of the base class\n> > > > + * - invoke the LIBCAMERA_DECLARE_PUBLIC() macro at the very top of the Private\n> > > > + * data class definition\n> > > > + * - pass a pointer to a newly allocated Private data object to the constructor\n> > > > + * of the base class\n> > > > + *\n> > > > + * Additionally, if the PublicClass is not final, it shall expose one or more\n> > > > + * constructors that takes a pointer to a Private data instance, to be used by\n> > > > + * derived classes.\n> > > > + *\n> > > > + * The Private class is fully opaque to users of the libcamera public API.\n> > > > + * Internally, it can be kept private to the implementation of PublicClass, or\n> > > > + * be exposed to other classes. In the latter case, the members of the Private\n> > > > + * class need to be qualified with appropriate access specifiers. The\n> > > > + * PublicClass and Private classes always have full access to each other's\n> > > > + * protected and private members.\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct an instance of an Extensible class\n> > > > + * \\param[in] d Pointer to the private data instance\n> > > > + */\n> > > > +Extensible::Extensible(Extensible::Private *d)\n> > > > +\t: d_(d)\n> > > > +{\n> > > > +}\n> > >\n> > > I wonder if we could avoid hainvg in the extensible derived classes\n> > >\n> > > : Extensible(new Private(...))\n> > >\n> > > by making Extensible accept a template argument pack that can be\n> > > forwarded to the construction of d_.\n> > >\n> > > I just wonder, I'm not proposing to try it :)\n> >\n> > I'm not sure to follow you, do you have an example ?\n> \n> Don't worry, I was just thinking out loud. Please ignore this.\n> \n> > > > +\n> > > > +/**\n> > > > + * \\var Extensible::d_\n> > > > + * \\brief Pointer to the private data instance\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\class Extensible::Private\n> > > > + * \\brief Base class for private data managed through a d-pointer\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct an instance of an Extensible class private data\n> > > > + * \\param[in] o Pointer to the public class object\n> > > > + */\n> > > > +Extensible::Private::Private(Extensible *o)\n> > > > +\t: o_(o)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +Extensible::Private::~Private()\n> > >\n> > > Why doesn't doxygen complain ?\n> >\n> > Because it contains this:\n> >\n> > EXCLUDE_SYMBOLS = libcamera::BoundMethodArgs \\\n> > libcamera::BoundMethodBase \\\n> > libcamera::BoundMethodMember \\\n> > libcamera::BoundMethodPack \\\n> > libcamera::BoundMethodPackBase \\\n> > libcamera::BoundMethodStatic \\\n> > libcamera::SignalBase \\\n> > libcamera::*::Private \\\n> > *::details::* \\\n> > std::*\n> \n> Ack.\n> \n> Overall I think I've pestered you enough. If you want to enforce usage\n> of 'd' and 'o' having macros to either declare the variable or just shorten\n> the access is fine. I still lean towards letting classes open code\n> access to private and public classes, without enforcing naming, but\n> everything is fine.\n> \n> For next version, with minors fixed:\n> Reviewed-by: Jacopo Mondi \n\nThank you :-)\n\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\var Extensible::Private::o_\n> > > > + * \\brief Pointer to the public class object\n> > > > + */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index 47ddb4014a61..b9f6457433f9 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -17,6 +17,7 @@ libcamera_sources = files([\n> > > > 'event_dispatcher.cpp',\n> > > > 'event_dispatcher_poll.cpp',\n> > > > 'event_notifier.cpp',\n> > > > + 'extensible.cpp',\n> > > > 'file.cpp',\n> > > > 'file_descriptor.cpp',\n> > > > 'flags.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7BD2BBDB1E\n\tfor ;\n\tTue, 27 Oct 2020 12:20:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F1EDF620C6;\n\tTue, 27 Oct 2020 13:20:03 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4EE262078\n\tfor ;\n\tTue, 27 Oct 2020 13:20:02 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66811BBE;\n\tTue, 27 Oct 2020 13:20:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"myU7XWbX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603801202;\n\tbh=6by+08ZNPBTdPgU1Zksq2D5SRjHQKMDAnTroBuNifZA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=myU7XWbXbJM4RQzTVOv/PTa+H05e/DrNb/fZ2icwbsWqvZJcXENbnq7Iw/YwNHa88\n\tGOWzTDdyZYhoFxyfkoCwQ47gUA90HUVOzIDEWQJBAYuS2vvEgNgYQcV7Pmb8f06mdP\n\tvrmo2xJdcTXE9ainZutieuxZY1S6+C5NSdGpZ2hg=","Date":"Tue, 27 Oct 2020 14:19:15 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201027121915.GA24666@pendragon.ideasonboard.com>","References":"<20201020014005.12783-1-laurent.pinchart@ideasonboard.com>\n\t<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>\n\t<20201021100412.2c572pwyw4xntsoi@uno.localdomain>\n\t<20201021225329.GQ3942@pendragon.ideasonboard.com>\n\t<20201027121106.hphsgsoioqfmakpy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201027121106.hphsgsoioqfmakpy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13517,"web_url":"https://patchwork.libcamera.org/comment/13517/","msgid":"<20201027140154.xfl5hzu2apkeuvll@uno.localdomain>","date":"2020-10-27T14:01:54","subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Oct 27, 2020 at 02:19:15PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Oct 27, 2020 at 01:11:06PM +0100, Jacopo Mondi wrote:\n\n[snip]\n\n> > > > In private class:\n> > > > ClassName *const asdsad = _public();\n> > > >\n> > > > without introducing macros that has to be followed and really just\n> > > > wrap one method call.\n> > >\n> > > The variable name isn't constrained by this patch, unlike v1 that hid\n> > > the variable declaration in the macro. I however want to keep the names\n> > > consistent, as that increases readability of the code base. Anyone\n> > > familiar with libcamera should be able to immediately understand what d\n> > > and o are (d comes from the d-pointer design pattern and o stands for\n> > > object, but I'm fine discussing different names, especially for o),\n> > > hence patch 1/5 in this series to enforce that rule.\n> >\n> > Yeah, I meant not reporting an error in checkstyle.\n> >\n> > > As for calling functions directly instead of using macros, I think the\n> > > macros make it more readable by hiding the implementation details.\n> >\n> > Ok, so, and sorry for backtracking, if you want 'd' and 'o' to be\n> > forced and get it as recognizible libcamera construct, macros are fine\n> > too\n>\n> Just to be sure, does this mean you prefer\n>\n> \tPrivate * const d = LIBCAMERA_D_PTR();\n>\n> or\n>\n> \tLIBCAMERA_D_PTR();\n>\n> with the d variable hidden in the macro ?\n\nI would prefer letting each implementation use the variable name they\nlike, without introducing any library specific pattern that increase\nthe entry barrier to grasp the code base.\n\nBut I see your point and if you want to force the usage of the 'o' and\n'd' variable names using checkstyle, I think the two are equivalent,\nwith a slight preference for the first that at least has the variable\nname explicit and not hidden behind a macro.\n\nUp to you, really.","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74C5BC3B5C\n\tfor ;\n\tTue, 27 Oct 2020 14:01:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DCDBE62109;\n\tTue, 27 Oct 2020 15:01:57 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0C4862034\n\tfor ;\n\tTue, 27 Oct 2020 15:01:56 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 3596A4000D;\n\tTue, 27 Oct 2020 14:01:55 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 27 Oct 2020 15:01:54 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201027140154.xfl5hzu2apkeuvll@uno.localdomain>","References":"<20201020014005.12783-1-laurent.pinchart@ideasonboard.com>\n\t<20201020014005.12783-3-laurent.pinchart@ideasonboard.com>\n\t<20201021100412.2c572pwyw4xntsoi@uno.localdomain>\n\t<20201021225329.GQ3942@pendragon.ideasonboard.com>\n\t<20201027121106.hphsgsoioqfmakpy@uno.localdomain>\n\t<20201027121915.GA24666@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201027121915.GA24666@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/5] libcamera: Add a base class to\n\timplement the d-pointer design pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]