[{"id":18299,"web_url":"https://patchwork.libcamera.org/comment/18299/","msgid":"","date":"2021-07-24T06:55:17","subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2021-07-23 07:00:22 +0300, Laurent Pinchart wrote:\n> The Extensible and Extensible::Private classes contain pointers to each\n> other. These pointers are initialized in the respective class's\n> constructor, by passing a pointer to the other class to each\n> constructor. This particular construct reduces the flexibility of the\n> Extensible pattern, as the Private class instance has to be allocated\n> and constructed in the members initializer list of the Extensible\n> class's constructor. It is thus impossible to perform any operation on\n> the Private class between its construction and the construction of the\n> Extensible class, or to subclass the Private class without subclassing\n> the Extensible class.\n> \n> To make the design pattern more flexible, don't pass the pointer to the\n> Extensible class to the Private class's constructor, but initialize the\n> pointer manually in the Extensible class's constructor. This requires a\n> const_cast as the o_ member of the Private class is const.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/class.h | 3 ++-\n> include/libcamera/internal/framebuffer.h | 2 +-\n> src/android/camera_hal_config.cpp | 7 +++----\n> src/android/mm/cros_camera_buffer.cpp | 4 ++--\n> src/android/mm/generic_camera_buffer.cpp | 3 +--\n> src/libcamera/base/class.cpp | 6 +++---\n> src/libcamera/camera.cpp | 10 +++++-----\n> src/libcamera/camera_manager.cpp | 8 ++++----\n> src/libcamera/framebuffer.cpp | 6 +++---\n> 9 files changed, 24 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> index 9c7f0f2e6e27..c9d9cd949ca1 100644\n> --- a/include/libcamera/base/class.h\n> +++ b/include/libcamera/base/class.h\n> @@ -64,7 +64,7 @@ public:\n> \tclass Private\n> \t{\n> \tpublic:\n> -\t\tPrivate(Extensible *o);\n> +\t\tPrivate();\n> \t\tvirtual ~Private();\n> \n> #ifndef __DOXYGEN__\n> @@ -82,6 +82,7 @@ public:\n> #endif\n> \n> \tprivate:\n> +\t\tfriend class Extensible;\n\nI don't like friends, but here it makes sens. I would add a comment here \nexplaining why the friend statement is needed so it can be removed in \nthe future if someone can think of a different design pattern. With this \nfixed,\n\nReviewed-by: Niklas Söderlund \n\n> \t\tExtensible *const o_;\n> \t};\n> \n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index a11e895d9b88..8c187adf70c7 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> \n> public:\n> -\tPrivate(FrameBuffer *buffer);\n> +\tPrivate();\n> \n> \tvoid setRequest(Request *request) { request_ = request; }\n> \n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> index 833cf4ba98a9..bbbb1410b52c 100644\n> --- a/src/android/camera_hal_config.cpp\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> \n> public:\n> -\tPrivate(CameraHalConfig *halConfig);\n> +\tPrivate();\n> \n> \tint parseConfigFile(FILE *fh, std::map *cameras);\n> \n> @@ -50,8 +50,7 @@ private:\n> \tstd::map *cameras_;\n> };\n> \n> -CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> -\t: Extensible::Private(halConfig)\n> +CameraHalConfig::Private::Private()\n> {\n> }\n> \n> @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> }\n> \n> CameraHalConfig::CameraHalConfig()\n> -\t: Extensible(new Private(this)), exists_(false), valid_(false)\n> +\t: Extensible(new Private()), exists_(false), valid_(false)\n> {\n> \tparseConfigurationFile();\n> }\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index bb55b95e3a39..437502fb8276 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -47,8 +47,8 @@ private:\n> CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> \t\t\t [[maybe_unused]] int flags)\n> -\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> -\t numPlanes_(0), valid_(false), registered_(false)\n> +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> +\t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> \n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 166be36efd5b..2a4b77ea5236 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -34,9 +34,8 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> };\n> \n> -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer, int flags)\n> -\t: Extensible::Private(cameraBuffer)\n> {\n> \tmaps_.reserve(camera3Buffer->numFds);\n> \terror_ = 0;\n> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp\n> index d4f0ac64ad48..d24043c20e55 100644\n> --- a/src/libcamera/base/class.cpp\n> +++ b/src/libcamera/base/class.cpp\n> @@ -151,6 +151,7 @@ namespace libcamera {\n> Extensible::Extensible(Extensible::Private *d)\n> \t: d_(d)\n> {\n> +\t*const_cast(&d_->o_) = this;\n> }\n> \n> /**\n> @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d)\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> +Extensible::Private::Private()\n> +\t: o_(nullptr)\n> {\n> }\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c8858e71d105..c126b49290ce 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -344,7 +344,7 @@ public:\n> \t\tCameraRunning,\n> \t};\n> \n> -\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n> +\tPrivate(PipelineHandler *pipe, const std::string &id,\n> \t\tconst std::set &streams);\n> \t~Private();\n> \n> @@ -368,11 +368,11 @@ private:\n> \tstd::atomic state_;\n> };\n> \n> -Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> +Camera::Private::Private(PipelineHandler *pipe,\n> \t\t\t const std::string &id,\n> \t\t\t const std::set &streams)\n> -\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> -\t streams_(streams), disconnected_(false), state_(CameraAvailable)\n> +\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> +\t disconnected_(false), state_(CameraAvailable)\n> {\n> }\n> \n> @@ -632,7 +632,7 @@ const std::string &Camera::id() const\n> \n> Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> \t const std::set &streams)\n> -\t: Extensible(new Private(this, pipe, id, streams))\n> +\t: Extensible(new Private(pipe, id, streams))\n> {\n> }\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 1c79308ad4b5..384a574f2baa 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraManager)\n> \n> public:\n> -\tPrivate(CameraManager *cm);\n> +\tPrivate();\n> \n> \tint start();\n> \tvoid addCamera(std::shared_ptr camera,\n> @@ -74,8 +74,8 @@ private:\n> \tProcessManager processManager_;\n> };\n> \n> -CameraManager::Private::Private(CameraManager *cm)\n> -\t: Extensible::Private(cm), initialized_(false)\n> +CameraManager::Private::Private()\n> +\t: initialized_(false)\n> {\n> }\n> \n> @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> CameraManager *CameraManager::self_ = nullptr;\n> \n> CameraManager::CameraManager()\n> -\t: Extensible(new CameraManager::Private(this))\n> +\t: Extensible(new CameraManager::Private())\n> {\n> \tif (self_)\n> \t\tLOG(Camera, Fatal)\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 40bf64b0a4fe..48d14b31f68d 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * \\brief Array of per-plane metadata\n> */\n> \n> -FrameBuffer::Private::Private(FrameBuffer *buffer)\n> -\t: Extensible::Private(buffer), request_(nullptr)\n> +FrameBuffer::Private::Private()\n> +\t: request_(nullptr)\n> {\n> }\n> \n> @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer)\n> * \\param[in] cookie Cookie\n> */\n> FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> -\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> +\t: Extensible(new Private()), planes_(planes), cookie_(cookie)\n> {\n> }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","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 9103CC322C\n\tfor ;\n\tSat, 24 Jul 2021 06:55:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4D6C687AC;\n\tSat, 24 Jul 2021 08:55:20 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 718A960273\n\tfor ;\n\tSat, 24 Jul 2021 08:55:19 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id u3so5749190lff.9\n\tfor ;\n\tFri, 23 Jul 2021 23:55:19 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\tbu16sm1271522lfb.271.2021.07.23.23.55.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 23 Jul 2021 23:55:18 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"vA6GyzVU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=zw9BT+xCzPqLMDkK4NwttpTNSE3S0pPJfwrjNl5yOLw=;\n\tb=vA6GyzVUQh0N3BgZxAsMNxHPgP9EC9fNtb49L8JvRKR6X6tErSQers/OsmurTmiHB4\n\tCdXu46vh/mGozHIyBlHC0pWD9Zzg94JxT0P8Xf7FRdv4srS4Cl4xsM8m0bLWno39zzzG\n\thxeb0cjRw1DLb5D0ku7q56/FOCqVrfbdRPiKnc6ZMo/OOPEKqzzjyhcjy9hWlCFafCpg\n\tp/1l3jEPEFIuaY+eRoU9Bo/8DqHPwmFwkIF5qQZJaG4hAdbRS+e5bwBQJwK3A7hCUC7x\n\t7Wx36bbdUJfE4JLl+kPRT1ZWb5jiBYn+Y6lXvOCLfuldTxBKPuI5df2B5p9d53UwufJA\n\tNPZg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=zw9BT+xCzPqLMDkK4NwttpTNSE3S0pPJfwrjNl5yOLw=;\n\tb=kKNg8fMPWXg+gsviumJZjoEK3n1Fv8Zt2WaMs9z6bWXZ2X1LQRuFryeY7iR7zdKLK5\n\th9T8ciQkN33H1ys38gjlEW8g62NU2lN7Ey66JAA+lbMPzv3MlL3Xr4aOEJMyjxW6zUVT\n\tmXq+GGksQ1py7VaaU3ATo6YUq5af0aI2sSJOXHeOCd1758W9Qf4bU7j/4CdBIBZvD92O\n\tgmFlzoxofYud2ykFLOdn873fHDDlD/AwJ8v3lfJqbWliXQsPGkC9RW4q7nNMlqJ+fqXM\n\toOCNPRs8ZgugLNqkdXTpruva9L5/bkKjWuYUl80NnZ63TShf7R1UPqfZ+BGAmWc3lVXt\n\t/CcA==","X-Gm-Message-State":"AOAM533fOgVT+YNd2PB4egZ3KMFBWAY5cDh2rt1Uts19zq3zUKaUVzgR\n\tjrqFxv3YhpQX+D6qL277CVWv7A==","X-Google-Smtp-Source":"ABdhPJxBlFqvK6l9OnRxxx9vHYlutABR8fpkAgNAQEHiCtG9ufD0VZc+uJmJCrxb5bxhcE8iU9NRvA==","X-Received":"by 2002:a19:c707:: with SMTP id x7mr5702075lff.38.1627109718745; \n\tFri, 23 Jul 2021 23:55:18 -0700 (PDT)","Date":"Sat, 24 Jul 2021 08:55:17 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18314,"web_url":"https://patchwork.libcamera.org/comment/18314/","msgid":"","date":"2021-07-24T18:53:35","subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Sat, Jul 24, 2021 at 08:55:17AM +0200, Niklas Söderlund wrote:\n> On 2021-07-23 07:00:22 +0300, Laurent Pinchart wrote:\n> > The Extensible and Extensible::Private classes contain pointers to each\n> > other. These pointers are initialized in the respective class's\n> > constructor, by passing a pointer to the other class to each\n> > constructor. This particular construct reduces the flexibility of the\n> > Extensible pattern, as the Private class instance has to be allocated\n> > and constructed in the members initializer list of the Extensible\n> > class's constructor. It is thus impossible to perform any operation on\n> > the Private class between its construction and the construction of the\n> > Extensible class, or to subclass the Private class without subclassing\n> > the Extensible class.\n> > \n> > To make the design pattern more flexible, don't pass the pointer to the\n> > Extensible class to the Private class's constructor, but initialize the\n> > pointer manually in the Extensible class's constructor. This requires a\n> > const_cast as the o_ member of the Private class is const.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/class.h | 3 ++-\n> > include/libcamera/internal/framebuffer.h | 2 +-\n> > src/android/camera_hal_config.cpp | 7 +++----\n> > src/android/mm/cros_camera_buffer.cpp | 4 ++--\n> > src/android/mm/generic_camera_buffer.cpp | 3 +--\n> > src/libcamera/base/class.cpp | 6 +++---\n> > src/libcamera/camera.cpp | 10 +++++-----\n> > src/libcamera/camera_manager.cpp | 8 ++++----\n> > src/libcamera/framebuffer.cpp | 6 +++---\n> > 9 files changed, 24 insertions(+), 25 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > index 9c7f0f2e6e27..c9d9cd949ca1 100644\n> > --- a/include/libcamera/base/class.h\n> > +++ b/include/libcamera/base/class.h\n> > @@ -64,7 +64,7 @@ public:\n> > \tclass Private\n> > \t{\n> > \tpublic:\n> > -\t\tPrivate(Extensible *o);\n> > +\t\tPrivate();\n> > \t\tvirtual ~Private();\n> > \n> > #ifndef __DOXYGEN__\n> > @@ -82,6 +82,7 @@ public:\n> > #endif\n> > \n> > \tprivate:\n> > +\t\tfriend class Extensible;\n> \n> I don't like friends, but here it makes sens. I would add a comment here \n> explaining why the friend statement is needed so it can be removed in \n> the future if someone can think of a different design pattern. With this \n> fixed,\n\nI'll add the following comment:\n\n\t\t/* To initialize o_ from Extensible. */\n\t\tfriend class Extensible;\n\t\tExtensible *const o_;\n\n> Reviewed-by: Niklas Söderlund \n> \n> > \t\tExtensible *const o_;\n> > \t};\n> > \n> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > index a11e895d9b88..8c187adf70c7 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private\n> > \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > \n> > public:\n> > -\tPrivate(FrameBuffer *buffer);\n> > +\tPrivate();\n> > \n> > \tvoid setRequest(Request *request) { request_ = request; }\n> > \n> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > index 833cf4ba98a9..bbbb1410b52c 100644\n> > --- a/src/android/camera_hal_config.cpp\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> > \n> > public:\n> > -\tPrivate(CameraHalConfig *halConfig);\n> > +\tPrivate();\n> > \n> > \tint parseConfigFile(FILE *fh, std::map *cameras);\n> > \n> > @@ -50,8 +50,7 @@ private:\n> > \tstd::map *cameras_;\n> > };\n> > \n> > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> > -\t: Extensible::Private(halConfig)\n> > +CameraHalConfig::Private::Private()\n> > {\n> > }\n> > \n> > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> > }\n> > \n> > CameraHalConfig::CameraHalConfig()\n> > -\t: Extensible(new Private(this)), exists_(false), valid_(false)\n> > +\t: Extensible(new Private()), exists_(false), valid_(false)\n> > {\n> > \tparseConfigurationFile();\n> > }\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index bb55b95e3a39..437502fb8276 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -47,8 +47,8 @@ private:\n> > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > \t\t\t buffer_handle_t camera3Buffer,\n> > \t\t\t [[maybe_unused]] int flags)\n> > -\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> > -\t numPlanes_(0), valid_(false), registered_(false)\n> > +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > +\t registered_(false)\n> > {\n> > \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> > \n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index 166be36efd5b..2a4b77ea5236 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -34,9 +34,8 @@ public:\n> > \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > };\n> > \n> > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > \t\t\t buffer_handle_t camera3Buffer, int flags)\n> > -\t: Extensible::Private(cameraBuffer)\n> > {\n> > \tmaps_.reserve(camera3Buffer->numFds);\n> > \terror_ = 0;\n> > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp\n> > index d4f0ac64ad48..d24043c20e55 100644\n> > --- a/src/libcamera/base/class.cpp\n> > +++ b/src/libcamera/base/class.cpp\n> > @@ -151,6 +151,7 @@ namespace libcamera {\n> > Extensible::Extensible(Extensible::Private *d)\n> > \t: d_(d)\n> > {\n> > +\t*const_cast(&d_->o_) = this;\n> > }\n> > \n> > /**\n> > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d)\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> > +Extensible::Private::Private()\n> > +\t: o_(nullptr)\n> > {\n> > }\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c8858e71d105..c126b49290ce 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -344,7 +344,7 @@ public:\n> > \t\tCameraRunning,\n> > \t};\n> > \n> > -\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n> > +\tPrivate(PipelineHandler *pipe, const std::string &id,\n> > \t\tconst std::set &streams);\n> > \t~Private();\n> > \n> > @@ -368,11 +368,11 @@ private:\n> > \tstd::atomic state_;\n> > };\n> > \n> > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> > +Camera::Private::Private(PipelineHandler *pipe,\n> > \t\t\t const std::string &id,\n> > \t\t\t const std::set &streams)\n> > -\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> > -\t streams_(streams), disconnected_(false), state_(CameraAvailable)\n> > +\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> > +\t disconnected_(false), state_(CameraAvailable)\n> > {\n> > }\n> > \n> > @@ -632,7 +632,7 @@ const std::string &Camera::id() const\n> > \n> > Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> > \t const std::set &streams)\n> > -\t: Extensible(new Private(this, pipe, id, streams))\n> > +\t: Extensible(new Private(pipe, id, streams))\n> > {\n> > }\n> > \n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index 1c79308ad4b5..384a574f2baa 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraManager)\n> > \n> > public:\n> > -\tPrivate(CameraManager *cm);\n> > +\tPrivate();\n> > \n> > \tint start();\n> > \tvoid addCamera(std::shared_ptr camera,\n> > @@ -74,8 +74,8 @@ private:\n> > \tProcessManager processManager_;\n> > };\n> > \n> > -CameraManager::Private::Private(CameraManager *cm)\n> > -\t: Extensible::Private(cm), initialized_(false)\n> > +CameraManager::Private::Private()\n> > +\t: initialized_(false)\n> > {\n> > }\n> > \n> > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> > CameraManager *CameraManager::self_ = nullptr;\n> > \n> > CameraManager::CameraManager()\n> > -\t: Extensible(new CameraManager::Private(this))\n> > +\t: Extensible(new CameraManager::Private())\n> > {\n> > \tif (self_)\n> > \t\tLOG(Camera, Fatal)\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index 40bf64b0a4fe..48d14b31f68d 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > * \\brief Array of per-plane metadata\n> > */\n> > \n> > -FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > -\t: Extensible::Private(buffer), request_(nullptr)\n> > +FrameBuffer::Private::Private()\n> > +\t: request_(nullptr)\n> > {\n> > }\n> > \n> > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > * \\param[in] cookie Cookie\n> > */\n> > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > -\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> > +\t: Extensible(new Private()), planes_(planes), cookie_(cookie)\n> > {\n> > }\n> >","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 5AB02C322C\n\tfor ;\n\tSat, 24 Jul 2021 18:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D4E4687AF;\n\tSat, 24 Jul 2021 20:53:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90DFE68536\n\tfor ;\n\tSat, 24 Jul 2021 20:53:39 +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 03CBD3D7;\n\tSat, 24 Jul 2021 20:53:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S9mPvO/4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627152819;\n\tbh=iuASmXBdmXVsxaPC93X6QhLiKsTpiGUhMyNDGdqMClU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S9mPvO/4yqyU/oCRK+sAWnWPzYQix7CkHvMlSp+Az/Ia/z+beG4I9e6eF0+7WlODa\n\tdCutAmDPNJVOblTzKdgR2PwB2HcFu8gEpkz6U/rA6PO5dWvd1SSbn3PrKOD8+vdUMX\n\t4bE60cldFIoK6zIhtbo/IKPSg+2AhHPHRb6BNDxg=","Date":"Sat, 24 Jul 2021 21:53:35 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18424,"web_url":"https://patchwork.libcamera.org/comment/18424/","msgid":"<20210729202318.wk5hzrugm3n3ev2m@uno.localdomain>","date":"2021-07-29T20:23:18","subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote:\n> The Extensible and Extensible::Private classes contain pointers to each\n> other. These pointers are initialized in the respective class's\n> constructor, by passing a pointer to the other class to each\n> constructor. This particular construct reduces the flexibility of the\n> Extensible pattern, as the Private class instance has to be allocated\n> and constructed in the members initializer list of the Extensible\n> class's constructor. It is thus impossible to perform any operation on\n> the Private class between its construction and the construction of the\n> Extensible class, or to subclass the Private class without subclassing\n\nAm I wrong or the main obstacle here is that is impossible to\nconstruct Private instances seprately from Public ones ?\n\n> the Extensible class.\n>\n> To make the design pattern more flexible, don't pass the pointer to the\n> Extensible class to the Private class's constructor, but initialize the\n> pointer manually in the Extensible class's constructor. This requires a\n> const_cast as the o_ member of the Private class is const.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/class.h | 3 ++-\n> include/libcamera/internal/framebuffer.h | 2 +-\n> src/android/camera_hal_config.cpp | 7 +++----\n> src/android/mm/cros_camera_buffer.cpp | 4 ++--\n> src/android/mm/generic_camera_buffer.cpp | 3 +--\n> src/libcamera/base/class.cpp | 6 +++---\n> src/libcamera/camera.cpp | 10 +++++-----\n> src/libcamera/camera_manager.cpp | 8 ++++----\n> src/libcamera/framebuffer.cpp | 6 +++---\n> 9 files changed, 24 insertions(+), 25 deletions(-)\n>\n> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> index 9c7f0f2e6e27..c9d9cd949ca1 100644\n> --- a/include/libcamera/base/class.h\n> +++ b/include/libcamera/base/class.h\n> @@ -64,7 +64,7 @@ public:\n> \tclass Private\n> \t{\n> \tpublic:\n> -\t\tPrivate(Extensible *o);\n> +\t\tPrivate();\n> \t\tvirtual ~Private();\n>\n> #ifndef __DOXYGEN__\n> @@ -82,6 +82,7 @@ public:\n> #endif\n>\n> \tprivate:\n> +\t\tfriend class Extensible;\n> \t\tExtensible *const o_;\n> \t};\n>\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index a11e895d9b88..8c187adf70c7 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n>\n> public:\n> -\tPrivate(FrameBuffer *buffer);\n> +\tPrivate();\n>\n> \tvoid setRequest(Request *request) { request_ = request; }\n>\n> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> index 833cf4ba98a9..bbbb1410b52c 100644\n> --- a/src/android/camera_hal_config.cpp\n> +++ b/src/android/camera_hal_config.cpp\n> @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n>\n> public:\n> -\tPrivate(CameraHalConfig *halConfig);\n> +\tPrivate();\n>\n> \tint parseConfigFile(FILE *fh, std::map *cameras);\n>\n> @@ -50,8 +50,7 @@ private:\n> \tstd::map *cameras_;\n> };\n>\n> -CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> -\t: Extensible::Private(halConfig)\n> +CameraHalConfig::Private::Private()\n> {\n> }\n>\n> @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> }\n>\n> CameraHalConfig::CameraHalConfig()\n> -\t: Extensible(new Private(this)), exists_(false), valid_(false)\n> +\t: Extensible(new Private()), exists_(false), valid_(false)\n> {\n> \tparseConfigurationFile();\n> }\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index bb55b95e3a39..437502fb8276 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -47,8 +47,8 @@ private:\n> CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer,\n> \t\t\t [[maybe_unused]] int flags)\n> -\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> -\t numPlanes_(0), valid_(false), registered_(false)\n> +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> +\t registered_(false)\n> {\n> \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n>\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 166be36efd5b..2a4b77ea5236 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -34,9 +34,8 @@ public:\n> \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> };\n>\n> -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> \t\t\t buffer_handle_t camera3Buffer, int flags)\n> -\t: Extensible::Private(cameraBuffer)\n> {\n> \tmaps_.reserve(camera3Buffer->numFds);\n> \terror_ = 0;\n> diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp\n> index d4f0ac64ad48..d24043c20e55 100644\n> --- a/src/libcamera/base/class.cpp\n> +++ b/src/libcamera/base/class.cpp\n> @@ -151,6 +151,7 @@ namespace libcamera {\n> Extensible::Extensible(Extensible::Private *d)\n> \t: d_(d)\n> {\n> +\t*const_cast(&d_->o_) = this;\n\nDo you think there's much value in\n Extensible *const o_;\n\nas it is private and only Extensible can access it ?\nIt will save the const_cast<> here (but according to my compiler will\nrequire a new one in the \"T * _o()\" function, so\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> }\n>\n> /**\n> @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d)\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> +Extensible::Private::Private()\n> +\t: o_(nullptr)\n> {\n> }\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c8858e71d105..c126b49290ce 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -344,7 +344,7 @@ public:\n> \t\tCameraRunning,\n> \t};\n>\n> -\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n> +\tPrivate(PipelineHandler *pipe, const std::string &id,\n> \t\tconst std::set &streams);\n> \t~Private();\n>\n> @@ -368,11 +368,11 @@ private:\n> \tstd::atomic state_;\n> };\n>\n> -Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> +Camera::Private::Private(PipelineHandler *pipe,\n> \t\t\t const std::string &id,\n> \t\t\t const std::set &streams)\n> -\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> -\t streams_(streams), disconnected_(false), state_(CameraAvailable)\n> +\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> +\t disconnected_(false), state_(CameraAvailable)\n> {\n> }\n>\n> @@ -632,7 +632,7 @@ const std::string &Camera::id() const\n>\n> Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> \t const std::set &streams)\n> -\t: Extensible(new Private(this, pipe, id, streams))\n> +\t: Extensible(new Private(pipe, id, streams))\n> {\n> }\n>\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 1c79308ad4b5..384a574f2baa 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread\n> \tLIBCAMERA_DECLARE_PUBLIC(CameraManager)\n>\n> public:\n> -\tPrivate(CameraManager *cm);\n> +\tPrivate();\n>\n> \tint start();\n> \tvoid addCamera(std::shared_ptr camera,\n> @@ -74,8 +74,8 @@ private:\n> \tProcessManager processManager_;\n> };\n>\n> -CameraManager::Private::Private(CameraManager *cm)\n> -\t: Extensible::Private(cm), initialized_(false)\n> +CameraManager::Private::Private()\n> +\t: initialized_(false)\n> {\n> }\n>\n> @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> CameraManager *CameraManager::self_ = nullptr;\n>\n> CameraManager::CameraManager()\n> -\t: Extensible(new CameraManager::Private(this))\n> +\t: Extensible(new CameraManager::Private())\n> {\n> \tif (self_)\n> \t\tLOG(Camera, Fatal)\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 40bf64b0a4fe..48d14b31f68d 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer)\n> * \\brief Array of per-plane metadata\n> */\n>\n> -FrameBuffer::Private::Private(FrameBuffer *buffer)\n> -\t: Extensible::Private(buffer), request_(nullptr)\n> +FrameBuffer::Private::Private()\n> +\t: request_(nullptr)\n> {\n> }\n>\n> @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer)\n> * \\param[in] cookie Cookie\n> */\n> FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> -\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> +\t: Extensible(new Private()), planes_(planes), cookie_(cookie)\n> {\n> }\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","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 B7A31C3230\n\tfor ;\n\tThu, 29 Jul 2021 20:22:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36177687BF;\n\tThu, 29 Jul 2021 22:22:35 +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 87519687BA\n\tfor ;\n\tThu, 29 Jul 2021 22:22:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6AE811BF20A;\n\tThu, 29 Jul 2021 20:22:32 +0000 (UTC)"],"Date":"Thu, 29 Jul 2021 22:23:18 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210729202318.wk5hzrugm3n3ev2m@uno.localdomain>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18433,"web_url":"https://patchwork.libcamera.org/comment/18433/","msgid":"","date":"2021-07-30T02:33:21","subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 29, 2021 at 10:23:18PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote:\n> > The Extensible and Extensible::Private classes contain pointers to each\n> > other. These pointers are initialized in the respective class's\n> > constructor, by passing a pointer to the other class to each\n> > constructor. This particular construct reduces the flexibility of the\n> > Extensible pattern, as the Private class instance has to be allocated\n> > and constructed in the members initializer list of the Extensible\n> > class's constructor. It is thus impossible to perform any operation on\n> > the Private class between its construction and the construction of the\n> > Extensible class, or to subclass the Private class without subclassing\n> \n> Am I wrong or the main obstacle here is that is impossible to\n> construct Private instances seprately from Public ones ?\n\nYes, before this patch both the public and private classes have to be\nconstructed together, with the constructor of the public class using, in\nits member initializer list,\n\n\t: Extensible(new Private(this))\n\nThe very tight coupling makes it impossible to use the Private class to\nstore private data ahead of construction of the public class.\n\n> > the Extensible class.\n> >\n> > To make the design pattern more flexible, don't pass the pointer to the\n> > Extensible class to the Private class's constructor, but initialize the\n> > pointer manually in the Extensible class's constructor. This requires a\n> > const_cast as the o_ member of the Private class is const.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/class.h | 3 ++-\n> > include/libcamera/internal/framebuffer.h | 2 +-\n> > src/android/camera_hal_config.cpp | 7 +++----\n> > src/android/mm/cros_camera_buffer.cpp | 4 ++--\n> > src/android/mm/generic_camera_buffer.cpp | 3 +--\n> > src/libcamera/base/class.cpp | 6 +++---\n> > src/libcamera/camera.cpp | 10 +++++-----\n> > src/libcamera/camera_manager.cpp | 8 ++++----\n> > src/libcamera/framebuffer.cpp | 6 +++---\n> > 9 files changed, 24 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > index 9c7f0f2e6e27..c9d9cd949ca1 100644\n> > --- a/include/libcamera/base/class.h\n> > +++ b/include/libcamera/base/class.h\n> > @@ -64,7 +64,7 @@ public:\n> > \tclass Private\n> > \t{\n> > \tpublic:\n> > -\t\tPrivate(Extensible *o);\n> > +\t\tPrivate();\n> > \t\tvirtual ~Private();\n> >\n> > #ifndef __DOXYGEN__\n> > @@ -82,6 +82,7 @@ public:\n> > #endif\n> >\n> > \tprivate:\n> > +\t\tfriend class Extensible;\n> > \t\tExtensible *const o_;\n> > \t};\n> >\n> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > index a11e895d9b88..8c187adf70c7 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private\n> > \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> >\n> > public:\n> > -\tPrivate(FrameBuffer *buffer);\n> > +\tPrivate();\n> >\n> > \tvoid setRequest(Request *request) { request_ = request; }\n> >\n> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > index 833cf4ba98a9..bbbb1410b52c 100644\n> > --- a/src/android/camera_hal_config.cpp\n> > +++ b/src/android/camera_hal_config.cpp\n> > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> >\n> > public:\n> > -\tPrivate(CameraHalConfig *halConfig);\n> > +\tPrivate();\n> >\n> > \tint parseConfigFile(FILE *fh, std::map *cameras);\n> >\n> > @@ -50,8 +50,7 @@ private:\n> > \tstd::map *cameras_;\n> > };\n> >\n> > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> > -\t: Extensible::Private(halConfig)\n> > +CameraHalConfig::Private::Private()\n> > {\n> > }\n> >\n> > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> > }\n> >\n> > CameraHalConfig::CameraHalConfig()\n> > -\t: Extensible(new Private(this)), exists_(false), valid_(false)\n> > +\t: Extensible(new Private()), exists_(false), valid_(false)\n> > {\n> > \tparseConfigurationFile();\n> > }\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index bb55b95e3a39..437502fb8276 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -47,8 +47,8 @@ private:\n> > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > \t\t\t buffer_handle_t camera3Buffer,\n> > \t\t\t [[maybe_unused]] int flags)\n> > -\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> > -\t numPlanes_(0), valid_(false), registered_(false)\n> > +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > +\t registered_(false)\n> > {\n> > \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> >\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index 166be36efd5b..2a4b77ea5236 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -34,9 +34,8 @@ public:\n> > \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > };\n> >\n> > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > \t\t\t buffer_handle_t camera3Buffer, int flags)\n> > -\t: Extensible::Private(cameraBuffer)\n> > {\n> > \tmaps_.reserve(camera3Buffer->numFds);\n> > \terror_ = 0;\n> > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp\n> > index d4f0ac64ad48..d24043c20e55 100644\n> > --- a/src/libcamera/base/class.cpp\n> > +++ b/src/libcamera/base/class.cpp\n> > @@ -151,6 +151,7 @@ namespace libcamera {\n> > Extensible::Extensible(Extensible::Private *d)\n> > \t: d_(d)\n> > {\n> > +\t*const_cast(&d_->o_) = this;\n> \n> Do you think there's much value in\n> Extensible *const o_;\n> \n> as it is private and only Extensible can access it ?\n\nDo you mean much value in keeping it const ? Or much value in having it\nat all ? If it's the latter, I'm not sure to understand the question. If\nit's the former, I've considered dropping the const qualifier, but it's\nnice to leverage the compiler to ensure the pointer isn't modified when\nit shouldn't.\n\n> It will save the const_cast<> here (but according to my compiler will\n> require a new one in the \"T * _o()\" function, so\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > }\n> >\n> > /**\n> > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d)\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> > +Extensible::Private::Private()\n> > +\t: o_(nullptr)\n> > {\n> > }\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c8858e71d105..c126b49290ce 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -344,7 +344,7 @@ public:\n> > \t\tCameraRunning,\n> > \t};\n> >\n> > -\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n> > +\tPrivate(PipelineHandler *pipe, const std::string &id,\n> > \t\tconst std::set &streams);\n> > \t~Private();\n> >\n> > @@ -368,11 +368,11 @@ private:\n> > \tstd::atomic state_;\n> > };\n> >\n> > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> > +Camera::Private::Private(PipelineHandler *pipe,\n> > \t\t\t const std::string &id,\n> > \t\t\t const std::set &streams)\n> > -\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> > -\t streams_(streams), disconnected_(false), state_(CameraAvailable)\n> > +\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> > +\t disconnected_(false), state_(CameraAvailable)\n> > {\n> > }\n> >\n> > @@ -632,7 +632,7 @@ const std::string &Camera::id() const\n> >\n> > Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> > \t const std::set &streams)\n> > -\t: Extensible(new Private(this, pipe, id, streams))\n> > +\t: Extensible(new Private(pipe, id, streams))\n> > {\n> > }\n> >\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index 1c79308ad4b5..384a574f2baa 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread\n> > \tLIBCAMERA_DECLARE_PUBLIC(CameraManager)\n> >\n> > public:\n> > -\tPrivate(CameraManager *cm);\n> > +\tPrivate();\n> >\n> > \tint start();\n> > \tvoid addCamera(std::shared_ptr camera,\n> > @@ -74,8 +74,8 @@ private:\n> > \tProcessManager processManager_;\n> > };\n> >\n> > -CameraManager::Private::Private(CameraManager *cm)\n> > -\t: Extensible::Private(cm), initialized_(false)\n> > +CameraManager::Private::Private()\n> > +\t: initialized_(false)\n> > {\n> > }\n> >\n> > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> > CameraManager *CameraManager::self_ = nullptr;\n> >\n> > CameraManager::CameraManager()\n> > -\t: Extensible(new CameraManager::Private(this))\n> > +\t: Extensible(new CameraManager::Private())\n> > {\n> > \tif (self_)\n> > \t\tLOG(Camera, Fatal)\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index 40bf64b0a4fe..48d14b31f68d 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > * \\brief Array of per-plane metadata\n> > */\n> >\n> > -FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > -\t: Extensible::Private(buffer), request_(nullptr)\n> > +FrameBuffer::Private::Private()\n> > +\t: request_(nullptr)\n> > {\n> > }\n> >\n> > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > * \\param[in] cookie Cookie\n> > */\n> > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > -\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> > +\t: Extensible(new Private()), planes_(planes), cookie_(cookie)\n> > {\n> > }\n> >","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 8037DC3230\n\tfor ;\n\tFri, 30 Jul 2021 02:33:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8BAA687BF;\n\tFri, 30 Jul 2021 04:33:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 130FD60506\n\tfor ;\n\tFri, 30 Jul 2021 04:33:30 +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 9353589B;\n\tFri, 30 Jul 2021 04:33:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IXPeh4YK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627612409;\n\tbh=b0bLLUvG/t4gubrvg5FYJaoywJatS1kXk9GXvAWDFjQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IXPeh4YKcsXq0ZT5MUlX0sQQ2h4V8ilXTbsELFRHWIUHB2u2bWQzMNwrZ3JLUARLm\n\ttRGV3ybSgVe46RHOX0XplQWqv6u5L/C2wMBnRVhJ+vJjuKYG7GCfV4rKWlRXpZpxU7\n\trVhQbddIUSgbAnsBLKfSdhZ+apGe7hyi1dIcfSBI=","Date":"Fri, 30 Jul 2021 05:33:21 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>\n\t<20210729202318.wk5hzrugm3n3ev2m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210729202318.wk5hzrugm3n3ev2m@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18439,"web_url":"https://patchwork.libcamera.org/comment/18439/","msgid":"<20210730072326.mjcu6rf6geptqlpk@uno.localdomain>","date":"2021-07-30T07:23:26","subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jul 30, 2021 at 05:33:21AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Jul 29, 2021 at 10:23:18PM +0200, Jacopo Mondi wrote:\n> > On Fri, Jul 23, 2021 at 07:00:22AM +0300, Laurent Pinchart wrote:\n> > > The Extensible and Extensible::Private classes contain pointers to each\n> > > other. These pointers are initialized in the respective class's\n> > > constructor, by passing a pointer to the other class to each\n> > > constructor. This particular construct reduces the flexibility of the\n> > > Extensible pattern, as the Private class instance has to be allocated\n> > > and constructed in the members initializer list of the Extensible\n> > > class's constructor. It is thus impossible to perform any operation on\n> > > the Private class between its construction and the construction of the\n> > > Extensible class, or to subclass the Private class without subclassing\n> >\n> > Am I wrong or the main obstacle here is that is impossible to\n> > construct Private instances seprately from Public ones ?\n>\n> Yes, before this patch both the public and private classes have to be\n> constructed together, with the constructor of the public class using, in\n> its member initializer list,\n>\n> \t: Extensible(new Private(this))\n>\n> The very tight coupling makes it impossible to use the Private class to\n> store private data ahead of construction of the public class.\n>\n> > > the Extensible class.\n> > >\n> > > To make the design pattern more flexible, don't pass the pointer to the\n> > > Extensible class to the Private class's constructor, but initialize the\n> > > pointer manually in the Extensible class's constructor. This requires a\n> > > const_cast as the o_ member of the Private class is const.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/base/class.h | 3 ++-\n> > > include/libcamera/internal/framebuffer.h | 2 +-\n> > > src/android/camera_hal_config.cpp | 7 +++----\n> > > src/android/mm/cros_camera_buffer.cpp | 4 ++--\n> > > src/android/mm/generic_camera_buffer.cpp | 3 +--\n> > > src/libcamera/base/class.cpp | 6 +++---\n> > > src/libcamera/camera.cpp | 10 +++++-----\n> > > src/libcamera/camera_manager.cpp | 8 ++++----\n> > > src/libcamera/framebuffer.cpp | 6 +++---\n> > > 9 files changed, 24 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > > index 9c7f0f2e6e27..c9d9cd949ca1 100644\n> > > --- a/include/libcamera/base/class.h\n> > > +++ b/include/libcamera/base/class.h\n> > > @@ -64,7 +64,7 @@ public:\n> > > \tclass Private\n> > > \t{\n> > > \tpublic:\n> > > -\t\tPrivate(Extensible *o);\n> > > +\t\tPrivate();\n> > > \t\tvirtual ~Private();\n> > >\n> > > #ifndef __DOXYGEN__\n> > > @@ -82,6 +82,7 @@ public:\n> > > #endif\n> > >\n> > > \tprivate:\n> > > +\t\tfriend class Extensible;\n> > > \t\tExtensible *const o_;\n> > > \t};\n> > >\n> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > index a11e895d9b88..8c187adf70c7 100644\n> > > --- a/include/libcamera/internal/framebuffer.h\n> > > +++ b/include/libcamera/internal/framebuffer.h\n> > > @@ -52,7 +52,7 @@ class FrameBuffer::Private : public Extensible::Private\n> > > \tLIBCAMERA_DECLARE_PUBLIC(FrameBuffer)\n> > >\n> > > public:\n> > > -\tPrivate(FrameBuffer *buffer);\n> > > +\tPrivate();\n> > >\n> > > \tvoid setRequest(Request *request) { request_ = request; }\n> > >\n> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp\n> > > index 833cf4ba98a9..bbbb1410b52c 100644\n> > > --- a/src/android/camera_hal_config.cpp\n> > > +++ b/src/android/camera_hal_config.cpp\n> > > @@ -32,7 +32,7 @@ class CameraHalConfig::Private : public Extensible::Private\n> > > \tLIBCAMERA_DECLARE_PUBLIC(CameraHalConfig)\n> > >\n> > > public:\n> > > -\tPrivate(CameraHalConfig *halConfig);\n> > > +\tPrivate();\n> > >\n> > > \tint parseConfigFile(FILE *fh, std::map *cameras);\n> > >\n> > > @@ -50,8 +50,7 @@ private:\n> > > \tstd::map *cameras_;\n> > > };\n> > >\n> > > -CameraHalConfig::Private::Private(CameraHalConfig *halConfig)\n> > > -\t: Extensible::Private(halConfig)\n> > > +CameraHalConfig::Private::Private()\n> > > {\n> > > }\n> > >\n> > > @@ -344,7 +343,7 @@ int CameraHalConfig::Private::parseConfigFile(FILE *fh,\n> > > }\n> > >\n> > > CameraHalConfig::CameraHalConfig()\n> > > -\t: Extensible(new Private(this)), exists_(false), valid_(false)\n> > > +\t: Extensible(new Private()), exists_(false), valid_(false)\n> > > {\n> > > \tparseConfigurationFile();\n> > > }\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index bb55b95e3a39..437502fb8276 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -47,8 +47,8 @@ private:\n> > > CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > > \t\t\t buffer_handle_t camera3Buffer,\n> > > \t\t\t [[maybe_unused]] int flags)\n> > > -\t: Extensible::Private(cameraBuffer), handle_(camera3Buffer),\n> > > -\t numPlanes_(0), valid_(false), registered_(false)\n> > > +\t: handle_(camera3Buffer), numPlanes_(0), valid_(false),\n> > > +\t registered_(false)\n> > > {\n> > > \tbufferManager_ = cros::CameraBufferManager::GetInstance();\n> > >\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index 166be36efd5b..2a4b77ea5236 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -34,9 +34,8 @@ public:\n> > > \tsize_t jpegBufferSize(size_t maxJpegBufferSize) const;\n> > > };\n> > >\n> > > -CameraBuffer::Private::Private(CameraBuffer *cameraBuffer,\n> > > +CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,\n> > > \t\t\t buffer_handle_t camera3Buffer, int flags)\n> > > -\t: Extensible::Private(cameraBuffer)\n> > > {\n> > > \tmaps_.reserve(camera3Buffer->numFds);\n> > > \terror_ = 0;\n> > > diff --git a/src/libcamera/base/class.cpp b/src/libcamera/base/class.cpp\n> > > index d4f0ac64ad48..d24043c20e55 100644\n> > > --- a/src/libcamera/base/class.cpp\n> > > +++ b/src/libcamera/base/class.cpp\n> > > @@ -151,6 +151,7 @@ namespace libcamera {\n> > > Extensible::Extensible(Extensible::Private *d)\n> > > \t: d_(d)\n> > > {\n> > > +\t*const_cast(&d_->o_) = this;\n> >\n> > Do you think there's much value in\n> > Extensible *const o_;\n> >\n> > as it is private and only Extensible can access it ?\n>\n> Do you mean much value in keeping it const ? Or much value in having it\n> at all ? If it's the latter, I'm not sure to understand the question. If\n> it's the former, I've considered dropping the const qualifier, but it's\n> nice to leverage the compiler to ensure the pointer isn't modified when\n> it shouldn't.\n\nI meant the const qualifier :)\n\nOnly the Extensible derived class could access o_ and as the newly\nintroduced const_cast shows, they can if they really want to. But ofc\nit makes it harder to do so, so I guess it has value.\n\n>\n> > It will save the const_cast<> here (but according to my compiler will\n> > require a new one in the \"T * _o()\" function, so\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > }\n> > >\n> > > /**\n> > > @@ -182,10 +183,9 @@ Extensible::Extensible(Extensible::Private *d)\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> > > +Extensible::Private::Private()\n> > > +\t: o_(nullptr)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c8858e71d105..c126b49290ce 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -344,7 +344,7 @@ public:\n> > > \t\tCameraRunning,\n> > > \t};\n> > >\n> > > -\tPrivate(Camera *camera, PipelineHandler *pipe, const std::string &id,\n> > > +\tPrivate(PipelineHandler *pipe, const std::string &id,\n> > > \t\tconst std::set &streams);\n> > > \t~Private();\n> > >\n> > > @@ -368,11 +368,11 @@ private:\n> > > \tstd::atomic state_;\n> > > };\n> > >\n> > > -Camera::Private::Private(Camera *camera, PipelineHandler *pipe,\n> > > +Camera::Private::Private(PipelineHandler *pipe,\n> > > \t\t\t const std::string &id,\n> > > \t\t\t const std::set &streams)\n> > > -\t: Extensible::Private(camera), pipe_(pipe->shared_from_this()), id_(id),\n> > > -\t streams_(streams), disconnected_(false), state_(CameraAvailable)\n> > > +\t: pipe_(pipe->shared_from_this()), id_(id), streams_(streams),\n> > > +\t disconnected_(false), state_(CameraAvailable)\n> > > {\n> > > }\n> > >\n> > > @@ -632,7 +632,7 @@ const std::string &Camera::id() const\n> > >\n> > > Camera::Camera(PipelineHandler *pipe, const std::string &id,\n> > > \t const std::set &streams)\n> > > -\t: Extensible(new Private(this, pipe, id, streams))\n> > > +\t: Extensible(new Private(pipe, id, streams))\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > > index 1c79308ad4b5..384a574f2baa 100644\n> > > --- a/src/libcamera/camera_manager.cpp\n> > > +++ b/src/libcamera/camera_manager.cpp\n> > > @@ -39,7 +39,7 @@ class CameraManager::Private : public Extensible::Private, public Thread\n> > > \tLIBCAMERA_DECLARE_PUBLIC(CameraManager)\n> > >\n> > > public:\n> > > -\tPrivate(CameraManager *cm);\n> > > +\tPrivate();\n> > >\n> > > \tint start();\n> > > \tvoid addCamera(std::shared_ptr camera,\n> > > @@ -74,8 +74,8 @@ private:\n> > > \tProcessManager processManager_;\n> > > };\n> > >\n> > > -CameraManager::Private::Private(CameraManager *cm)\n> > > -\t: Extensible::Private(cm), initialized_(false)\n> > > +CameraManager::Private::Private()\n> > > +\t: initialized_(false)\n> > > {\n> > > }\n> > >\n> > > @@ -258,7 +258,7 @@ void CameraManager::Private::removeCamera(Camera *camera)\n> > > CameraManager *CameraManager::self_ = nullptr;\n> > >\n> > > CameraManager::CameraManager()\n> > > -\t: Extensible(new CameraManager::Private(this))\n> > > +\t: Extensible(new CameraManager::Private())\n> > > {\n> > > \tif (self_)\n> > > \t\tLOG(Camera, Fatal)\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index 40bf64b0a4fe..48d14b31f68d 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -100,8 +100,8 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > > * \\brief Array of per-plane metadata\n> > > */\n> > >\n> > > -FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > > -\t: Extensible::Private(buffer), request_(nullptr)\n> > > +FrameBuffer::Private::Private()\n> > > +\t: request_(nullptr)\n> > > {\n> > > }\n> > >\n> > > @@ -176,7 +176,7 @@ FrameBuffer::Private::Private(FrameBuffer *buffer)\n> > > * \\param[in] cookie Cookie\n> > > */\n> > > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > -\t: Extensible(new Private(this)), planes_(planes), cookie_(cookie)\n> > > +\t: Extensible(new Private()), planes_(planes), cookie_(cookie)\n> > > {\n> > > }\n> > >\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 A65A8C322E\n\tfor ;\n\tFri, 30 Jul 2021 07:22:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDD62687BF;\n\tFri, 30 Jul 2021 09:22:41 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B3829687B1\n\tfor ;\n\tFri, 30 Jul 2021 09:22:40 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 00716200009;\n\tFri, 30 Jul 2021 07:22:39 +0000 (UTC)"],"Date":"Fri, 30 Jul 2021 09:23:26 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210730072326.mjcu6rf6geptqlpk@uno.localdomain>","References":"<20210723040036.32346-1-laurent.pinchart@ideasonboard.com>\n\t<20210723040036.32346-4-laurent.pinchart@ideasonboard.com>\n\t<20210729202318.wk5hzrugm3n3ev2m@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/17] libcamera: base: class:\n\tDon't pass Extensible pointer to Private constructor","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]