[{"id":17971,"web_url":"https://patchwork.libcamera.org/comment/17971/","msgid":"<20210705102934.4obvoedlb7bwbfeb@uno.localdomain>","date":"2021-07-05T10:29:34","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 05, 2021 at 02:28:15AM +0300, Laurent Pinchart wrote:\n> Despite sharing the same name, the private data class created by the\n> Extensible design pattern and the C++ private access specifier have\n> different goals. The latter specifies class members private to the\n> class, while the former stores data not visible to the application.\n>\n> There are use cases for accessing the private data class from other\n> classes inside libcamera. Make this possible by exposing public _d()\n> functions in the class deriving from Extensible. This won't allow access\n> to the private data by applications as the definition of the Private\n> class isn't visible outside of libcamera.\n\nNeat, but how can other internal class access the klass::Private\nimplementation which is defined in the .cpp file only ?\n\n>\n> The _d() functions need to be defined as template functions to delay\n> their evaluation, as the static_cast() operator in the Extensible::_d()\n> functions needs the Private class to be fully defined.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/class.h | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> index a07dac057331..8212c3d4a5ae 100644\n> --- a/include/libcamera/base/class.h\n> +++ b/include/libcamera/base/class.h\n> @@ -33,14 +33,24 @@ namespace libcamera {\n> #define LIBCAMERA_DECLARE_PRIVATE()\t\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> +\tfriend class Private;\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n> +\tconst Private *_d() const\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n> +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\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> #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> -\t_d();\n> +\t_d();\n>\n> #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> \t_o();\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 8BDCCBD794\n\tfor ;\n\tMon, 5 Jul 2021 10:28:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 18655684F9;\n\tMon, 5 Jul 2021 12:28:47 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFF286050A\n\tfor ;\n\tMon, 5 Jul 2021 12:28:45 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 5FEC6100008;\n\tMon, 5 Jul 2021 10:28:45 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 12:29:34 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210705102934.4obvoedlb7bwbfeb@uno.localdomain>","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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":17974,"web_url":"https://patchwork.libcamera.org/comment/17974/","msgid":"","date":"2021-07-05T10:38:16","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 12:29:34PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 05, 2021 at 02:28:15AM +0300, Laurent Pinchart wrote:\n> > Despite sharing the same name, the private data class created by the\n> > Extensible design pattern and the C++ private access specifier have\n> > different goals. The latter specifies class members private to the\n> > class, while the former stores data not visible to the application.\n> >\n> > There are use cases for accessing the private data class from other\n> > classes inside libcamera. Make this possible by exposing public _d()\n> > functions in the class deriving from Extensible. This won't allow access\n> > to the private data by applications as the definition of the Private\n> > class isn't visible outside of libcamera.\n> \n> Neat, but how can other internal class access the klass::Private\n> implementation which is defined in the .cpp file only ?\n\nPlease see 3/3 :-)\n\n> > The _d() functions need to be defined as template functions to delay\n> > their evaluation, as the static_cast() operator in the Extensible::_d()\n> > functions needs the Private class to be fully defined.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/class.h | 14 ++++++++++++--\n> > 1 file changed, 12 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > index a07dac057331..8212c3d4a5ae 100644\n> > --- a/include/libcamera/base/class.h\n> > +++ b/include/libcamera/base/class.h\n> > @@ -33,14 +33,24 @@ namespace libcamera {\n> > #define LIBCAMERA_DECLARE_PRIVATE()\t\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> > +\tfriend class Private;\t\t\t\t\t\t\\\n> > +\ttemplate \t\t\t\t\t\\\n> > +\tconst Private *_d() const\t\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn Extensible::_d();\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\ttemplate \t\t\t\t\t\\\n> > +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn Extensible::_d();\t\t\t\\\n> > +\t}\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> > #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> > -\t_d();\n> > +\t_d();\n> >\n> > #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> > \t_o();","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 54941BD794\n\tfor ;\n\tMon, 5 Jul 2021 10:39:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB6B4684F9;\n\tMon, 5 Jul 2021 12:38:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 302B56050A\n\tfor ;\n\tMon, 5 Jul 2021 12:38:59 +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 ABB08E7;\n\tMon, 5 Jul 2021 12:38:58 +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=\"wiY0HWXt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625481538;\n\tbh=LXvcBvt011ZdumyF5SF9NsUAxcRY6oJCq91U8JEvb14=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wiY0HWXt81dGXuCONpwYa46GfifA8MLvBQSanGAuHEwtudIRDXzNKjrbOPKJpnQuy\n\t9ARafpqaeTJ1JJh3CJHPvACNT+7Qz6EvPxPN4uqKaGqOnXEzgicYuf3192wsSSPP8Z\n\tEG595ExCOYR5MihPjvds6I+M+o9UsWHIO3QMT6iY=","Date":"Mon, 5 Jul 2021 13:38:16 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>\n\t<20210705102934.4obvoedlb7bwbfeb@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210705102934.4obvoedlb7bwbfeb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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":17994,"web_url":"https://patchwork.libcamera.org/comment/17994/","msgid":"","date":"2021-07-06T05:08:47","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 7/5/21 4:58 AM, Laurent Pinchart wrote:\n> Despite sharing the same name, the private data class created by the\n> Extensible design pattern and the C++ private access specifier have\n> different goals. The latter specifies class members private to the\n> class, while the former stores data not visible to the application.\n>\n> There are use cases for accessing the private data class from other\n> classes inside libcamera. Make this possible by exposing public _d()\n> functions in the class deriving from Extensible. This won't allow access\n> to the private data by applications as the definition of the Private\n> class isn't visible outside of libcamera.\nI like this idea.\n> The _d() functions need to be defined as template functions to delay\n> their evaluation, as the static_cast() operator in the Extensible::_d()\n> functions needs the Private class to be fully defined.\n>\n> Signed-off-by: Laurent Pinchart \n  Reviewed-by: Umang Jain \n> ---\n> include/libcamera/base/class.h | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> index a07dac057331..8212c3d4a5ae 100644\n> --- a/include/libcamera/base/class.h\n> +++ b/include/libcamera/base/class.h\n> @@ -33,14 +33,24 @@ namespace libcamera {\n> #define LIBCAMERA_DECLARE_PRIVATE()\t\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> +\tfriend class Private;\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n> +\tconst Private *_d() const\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n> +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\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> #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> -\t_d();\n> +\t_d();\n> \n> #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> \t_o();","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 8AE4CC3224\n\tfor ;\n\tTue, 6 Jul 2021 05:08:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E909A684F9;\n\tTue, 6 Jul 2021 07:08:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9551C60284\n\tfor ;\n\tTue, 6 Jul 2021 07:08:52 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.59])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3E72C3F5;\n\tTue, 6 Jul 2021 07:08:50 +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=\"bhtvqYWD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625548132;\n\tbh=gB2XmDtEaoI79G/ol2hCVQ4k+zkxXDAgexHe5zxYCfE=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=bhtvqYWD1CXWO1b8ObeymvLk7X0OTfOpbiL5hEFbHv1HDE4kHh1RE6y2euFM3OnZe\n\tWZ3zsBn/r2ighOG0QcIAG6Szki3TIGtA+QORwe4zR6m+vD4NTbZEGKbCmm2PFGB7u2\n\tCrOz/tgNV9X3hDU9PxRBs8GS3VQJ3CCeiTDRuxo8=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 6 Jul 2021 10:38:47 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18001,"web_url":"https://patchwork.libcamera.org/comment/18001/","msgid":"","date":"2021-07-07T08:24:23","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Tue, Jul 6, 2021 at 2:08 PM Umang Jain wrote:\n>\n> Hi Laurent,\n>\n> On 7/5/21 4:58 AM, Laurent Pinchart wrote:\n> > Despite sharing the same name, the private data class created by the\n> > Extensible design pattern and the C++ private access specifier have\n> > different goals. The latter specifies class members private to the\n> > class, while the former stores data not visible to the application.\n> >\n> > There are use cases for accessing the private data class from other\n> > classes inside libcamera. Make this possible by exposing public _d()\n> > functions in the class deriving from Extensible. This won't allow access\n> > to the private data by applications as the definition of the Private\n> > class isn't visible outside of libcamera.\n> I like this idea.\n> > The _d() functions need to be defined as template functions to delay\n> > their evaluation, as the static_cast() operator in the Extensible::_d()\n> > functions needs the Private class to be fully defined.\n> >\n> > Signed-off-by: Laurent Pinchart \n> Reviewed-by: Umang Jain \n\nReviewed-by: Hirokazu Honda \n> > ---\n> > include/libcamera/base/class.h | 14 ++++++++++++--\n> > 1 file changed, 12 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > index a07dac057331..8212c3d4a5ae 100644\n> > --- a/include/libcamera/base/class.h\n> > +++ b/include/libcamera/base/class.h\n> > @@ -33,14 +33,24 @@ namespace libcamera {\n> > #define LIBCAMERA_DECLARE_PRIVATE() \\\n> > public: \\\n> > class Private; \\\n> > - friend class Private;\n> > + friend class Private; \\\n> > + template \\\n> > + const Private *_d() const \\\n> > + { \\\n> > + return Extensible::_d(); \\\n> > + } \\\n> > + template \\\n> > + Private *_d() \\\n> > + { \\\n> > + return Extensible::_d(); \\\n> > + }\n> >\n> > #define LIBCAMERA_DECLARE_PUBLIC(klass) \\\n> > friend class klass; \\\n> > using Public = klass;\n> >\n> > #define LIBCAMERA_D_PTR() \\\n> > - _d();\n> > + _d();\n> >\n> > #define LIBCAMERA_O_PTR() \\\n> > _o();","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 57F96BD794\n\tfor ;\n\tWed, 7 Jul 2021 08:24:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 853AF68505;\n\tWed, 7 Jul 2021 10:24:36 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF7DC684E6\n\tfor ;\n\tWed, 7 Jul 2021 10:24:34 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id gb6so1965371ejc.5\n\tfor ;\n\tWed, 07 Jul 2021 01:24:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fWpQKJdl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ljVPy/52j7vNbwqrgs+XJH+FEEpg+EybUp3JdeFga3Y=;\n\tb=fWpQKJdlfiCghOOkGP2S0WFuhgpKXRw4QX62Xq0vE9Q07UpuVfEib4wxUbUWKUx605\n\tE/EaxoFadoD5oRbPhg6PTjjctb4WQ8NjxLoQjfTCSmyDvUwu57T5BqPZKW0oRAlVrPzB\n\td9yq9RgqY8gqIzKQ2fLa9xIXj45UfhblKpQuE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ljVPy/52j7vNbwqrgs+XJH+FEEpg+EybUp3JdeFga3Y=;\n\tb=bRdJFkjWAl8b9NNWN4uBx6TxtB9GSmlfIbRrV3Jayf8OzggwpTmHuilHFfBUqTjThm\n\t9nb1KvMoKT1lHbSIBLSe09F+KEW4CkquHBtv2HyXHALQpzLJ+P2ViAE5+N+7/lAgsLkJ\n\t7r9O4ya3ckPxJ33S28+tTDDETmsBu0IpvUqa9xxiY1FFZuE+PAWf38HyKKSjoAVjepJq\n\tWEOYCRWPUI57KAriWcUuSP/MD523G/jmT7Acadh90UjkNeFNYd/Lqp6phEcBNs8IJBWH\n\tv+oX+B4f3YUBTG3Ax4aaz766kZ7TfAry1WxXol3iU3doRuhOTMGCmh9qDtvSY9BG5td0\n\t2/nQ==","X-Gm-Message-State":"AOAM5321GdBukjlJ7ZLoLR1FxdKqQ/QfGPIrp/WgPNCMxqqjxkLqAKw/\n\tuWBZjZ82UT+wpTsAJto8pggodF1whd+LYb+0QlyM+w==","X-Google-Smtp-Source":"ABdhPJyMjCcx2SEeKBmUCwIwirvDrEeTH6nbvm0tBLNZ9yWJjTll9h824gz0rf59W/+m7f9iM4PZ9QMnA6THf8N6p/E=","X-Received":"by 2002:a17:906:6cf:: with SMTP id\n\tv15mr5161105ejb.306.1625646274224; \n\tWed, 07 Jul 2021 01:24:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 7 Jul 2021 17:24:23 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18004,"web_url":"https://patchwork.libcamera.org/comment/18004/","msgid":"<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>","date":"2021-07-07T08:55:18","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 05/07/2021 00:28, Laurent Pinchart wrote:\n> Despite sharing the same name, the private data class created by the\n> Extensible design pattern and the C++ private access specifier have\n> different goals. The latter specifies class members private to the\n> class, while the former stores data not visible to the application.\n> \n> There are use cases for accessing the private data class from other\n> classes inside libcamera. Make this possible by exposing public _d()\n> functions in the class deriving from Extensible. This won't allow access\n> to the private data by applications as the definition of the Private\n> class isn't visible outside of libcamera.\n\nIt almost makes me think the name for the D ptr type should be\n'Internal', or 'Protected' rather than 'Private', as it's not private\n(it can be accessed) - it's just simply not exposed ...\n\nBut I'm not opposed to keeping the names we have.\nUltimately it's the same concept [private: means private to the class,\nPrivate means private to libcamera as a whole]\n\n\n> The _d() functions need to be defined as template functions to delay\n> their evaluation, as the static_cast() operator in the Extensible::_d()\n> functions needs the Private class to be fully defined.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/base/class.h | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> index a07dac057331..8212c3d4a5ae 100644\n> --- a/include/libcamera/base/class.h\n> +++ b/include/libcamera/base/class.h\n> @@ -33,14 +33,24 @@ namespace libcamera {\n> #define LIBCAMERA_DECLARE_PRIVATE()\t\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> +\tfriend class Private;\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n\nIs this some template magic (hack?), to make it a template without\nactually needing to be a template or have any template parameters?\n\nIf so it would be nice to state that in a comment so readers don't\nwonder or query it.\n\n\n> +\tconst Private *_d() const\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\t\t\t\t\t\t\t\t\\\n> +\ttemplate \t\t\t\t\t\\\n> +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> +\t{\t\t\t\t\t\t\t\t\\\n> +\t\treturn Extensible::_d();\t\t\t\\\n> +\t}\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> #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> -\t_d();\n> +\t_d();\n> \n> #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> \t_o();\n> \n\nPresumably there wouldn't ever be the need for the equivalent _o() as\nthat will only every be used from internally in the Private class.\n\n--\nKieran","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 A54E7BD794\n\tfor ;\n\tWed, 7 Jul 2021 08:55:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A1DB68505;\n\tWed, 7 Jul 2021 10:55:22 +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 4487F684E6\n\tfor ;\n\tWed, 7 Jul 2021 10:55:21 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BCB142E4;\n\tWed, 7 Jul 2021 10:55:20 +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=\"pbAwYB5g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625648120;\n\tbh=ddrqTfkP1HiXB6eHPm4WmRmwnFDfm38u0lJiFGqRMjU=;\n\th=From:To:References:Subject:Date:In-Reply-To:From;\n\tb=pbAwYB5gi5V8NgR0BZQZP0ZfMSFahFkkLVskz6aimBwBEHh3JVMCoGh9z5C2PyxgL\n\tzcFD6i1p0k/Y32nvSXj5fV3MrOYDnnYUJmTcEwwe1mqTUbybfW0YORif6BlNmsHG3j\n\t5x6RuAHRSMPZTBFrOqCvlZMSWi+8CsFh1CMDdmS4=","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","Message-ID":"<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>","Date":"Wed, 7 Jul 2021 09:55:18 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18011,"web_url":"https://patchwork.libcamera.org/comment/18011/","msgid":"","date":"2021-07-07T11:46:35","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote:\n> On 05/07/2021 00:28, Laurent Pinchart wrote:\n> > Despite sharing the same name, the private data class created by the\n> > Extensible design pattern and the C++ private access specifier have\n> > different goals. The latter specifies class members private to the\n> > class, while the former stores data not visible to the application.\n> > \n> > There are use cases for accessing the private data class from other\n> > classes inside libcamera. Make this possible by exposing public _d()\n> > functions in the class deriving from Extensible. This won't allow access\n> > to the private data by applications as the definition of the Private\n> > class isn't visible outside of libcamera.\n> \n> It almost makes me think the name for the D ptr type should be\n> 'Internal', or 'Protected' rather than 'Private', as it's not private\n> (it can be accessed) - it's just simply not exposed ...\n\nI knew there would be a naming discussion :-)\n\nThe pointer is private to the library, but not private to the class (at\nleast not in all cases, a particular class doesn't *have* to expose its\nPrivate class if there's no need too). And it's internal to the library\ntoo, but not internal to the class.\n\n> But I'm not opposed to keeping the names we have.\n> Ultimately it's the same concept [private: means private to the class,\n> Private means private to libcamera as a whole]\n\nI should read through before answering :-)\n\n> > The _d() functions need to be defined as template functions to delay\n> > their evaluation, as the static_cast() operator in the Extensible::_d()\n> > functions needs the Private class to be fully defined.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/base/class.h | 14 ++++++++++++--\n> > 1 file changed, 12 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> > index a07dac057331..8212c3d4a5ae 100644\n> > --- a/include/libcamera/base/class.h\n> > +++ b/include/libcamera/base/class.h\n> > @@ -33,14 +33,24 @@ namespace libcamera {\n> > #define LIBCAMERA_DECLARE_PRIVATE()\t\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> > +\tfriend class Private;\t\t\t\t\t\t\\\n> > +\ttemplate \t\t\t\t\t\\\n> \n> Is this some template magic (hack?), to make it a template without\n> actually needing to be a template or have any template parameters?\n> \n> If so it would be nice to state that in a comment so readers don't\n> wonder or query it.\n\nIt's explained in the last paragraph of the commit message, would you\nlike a comment here too ?\n\n> > +\tconst Private *_d() const\t\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn Extensible::_d();\t\t\t\\\n> > +\t}\t\t\t\t\t\t\t\t\\\n> > +\ttemplate \t\t\t\t\t\\\n> > +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> > +\t{\t\t\t\t\t\t\t\t\\\n> > +\t\treturn Extensible::_d();\t\t\t\\\n> > +\t}\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> > #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> > -\t_d();\n> > +\t_d();\n> > \n> > #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> > \t_o();\n> \n> Presumably there wouldn't ever be the need for the equivalent _o() as\n> that will only every be used from internally in the Private class.\n\nCorrect.","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 DCAC3BD794\n\tfor ;\n\tWed, 7 Jul 2021 11:47:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38D3A68503;\n\tWed, 7 Jul 2021 13:47:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9224E60284\n\tfor ;\n\tWed, 7 Jul 2021 13:47:20 +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 142DF2E4;\n\tWed, 7 Jul 2021 13:47:20 +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=\"KxyhPolj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625658440;\n\tbh=xgmQz4psBqRor+Xpst01MS/JEpb7b4+v5oP2n/4N10Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KxyhPoljgl5lus3PufH/kiJhh+T2eEhDr4FVm1dHFmnmPN1CcBqb+dFR5vb8DTlYw\n\tQDv87AaNyh51FoqkxAHh0V/+V1kgnmovCFXoVVtpfAQHgUq2jkTt5CeLc/5iGlPd6M\n\trBKnXGNV7y4UA11B4upRweOc7kMvnZKthk37f82w=","Date":"Wed, 7 Jul 2021 14:46:35 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>\n\t<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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":18014,"web_url":"https://patchwork.libcamera.org/comment/18014/","msgid":"<76564e10-de64-9ab1-ffc1-99e767341fb2@ideasonboard.com>","date":"2021-07-07T12:26:58","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 07/07/2021 12:46, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote:\n>> On 05/07/2021 00:28, Laurent Pinchart wrote:\n>>> Despite sharing the same name, the private data class created by the\n>>> Extensible design pattern and the C++ private access specifier have\n>>> different goals. The latter specifies class members private to the\n>>> class, while the former stores data not visible to the application.\n>>>\n>>> There are use cases for accessing the private data class from other\n>>> classes inside libcamera. Make this possible by exposing public _d()\n>>> functions in the class deriving from Extensible. This won't allow access\n>>> to the private data by applications as the definition of the Private\n>>> class isn't visible outside of libcamera.\n>>\n>> It almost makes me think the name for the D ptr type should be\n>> 'Internal', or 'Protected' rather than 'Private', as it's not private\n>> (it can be accessed) - it's just simply not exposed ...\n> \n> I knew there would be a naming discussion :-)\n> \n> The pointer is private to the library, but not private to the class (at\n> least not in all cases, a particular class doesn't *have* to expose its\n> Private class if there's no need too). And it's internal to the library\n> too, but not internal to the class.\n> \n>> But I'm not opposed to keeping the names we have.\n>> Ultimately it's the same concept [private: means private to the class,\n>> Private means private to libcamera as a whole]\n> \n> I should read through before answering :-)\n> \n>>> The _d() functions need to be defined as template functions to delay\n>>> their evaluation, as the static_cast() operator in the Extensible::_d()\n>>> functions needs the Private class to be fully defined.\n>>>\n>>> Signed-off-by: Laurent Pinchart \n>>> ---\n>>> include/libcamera/base/class.h | 14 ++++++++++++--\n>>> 1 file changed, 12 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n>>> index a07dac057331..8212c3d4a5ae 100644\n>>> --- a/include/libcamera/base/class.h\n>>> +++ b/include/libcamera/base/class.h\n>>> @@ -33,14 +33,24 @@ namespace libcamera {\n>>> #define LIBCAMERA_DECLARE_PRIVATE()\t\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>>> +\tfriend class Private;\t\t\t\t\t\t\\\n>>> +\ttemplate \t\t\t\t\t\\\n>>\n>> Is this some template magic (hack?), to make it a template without\n>> actually needing to be a template or have any template parameters?\n>>\n>> If so it would be nice to state that in a comment so readers don't\n>> wonder or query it.\n> \n> It's explained in the last paragraph of the commit message, would you\n> like a comment here too ?\n\nThe commit message explains that we need to use templates to delay\nevaluation.\n\nBut looks like a magic hack, and otherwise out of place.\n\nReading this file without the commit message, it's really unclear why\nthere is a bool B = true, or even what it's supposed to do. (or more\nimportantly, that it's supposed to be simply ignored)\n\n\n>>> +\tconst Private *_d() const\t\t\t\t\t\\\n>>> +\t{\t\t\t\t\t\t\t\t\\\n>>> +\t\treturn Extensible::_d();\t\t\t\\\n>>> +\t}\t\t\t\t\t\t\t\t\\\n>>> +\ttemplate \t\t\t\t\t\\\n>>> +\tPrivate *_d()\t\t\t\t\t\t\t\\\n>>> +\t{\t\t\t\t\t\t\t\t\\\n>>> +\t\treturn Extensible::_d();\t\t\t\\\n>>> +\t}\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>>> #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n>>> -\t_d();\n>>> +\t_d();\n>>> \n>>> #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n>>> \t_o();\n>>\n>> Presumably there wouldn't ever be the need for the equivalent _o() as\n>> that will only every be used from internally in the Private class.\n> \n> Correct.\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 1E838C3224\n\tfor ;\n\tWed, 7 Jul 2021 12:27:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7CA9368503;\n\tWed, 7 Jul 2021 14:27:02 +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 18DBE60284\n\tfor ;\n\tWed, 7 Jul 2021 14:27:01 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91A5A2E4;\n\tWed, 7 Jul 2021 14:27:00 +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=\"hsAqgiJ6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625660820;\n\tbh=GfYOaRTAtv3j2GhpdqOfZY/OV2zmbhQB+04PMwzj7Ow=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=hsAqgiJ61HL1cZPIZXbrxl86vg84B4b9PoboHAKl18d9A6r0yZAy+0539lAZQ8aRF\n\t02u2/EBG8rteAp+BTkgSTdUhevpqB4pjtuxz5d8pgeQ95QRHj1Pga7QEP7kJZFUgNz\n\trp8HSg6jndUCYrnaMoOM21Z3+NjliVDKqNUuRdmI=","From":"Kieran Bingham ","To":"Laurent Pinchart ","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>\n\t<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>\n\t","Message-ID":"<76564e10-de64-9ab1-ffc1-99e767341fb2@ideasonboard.com>","Date":"Wed, 7 Jul 2021 13:26:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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":18075,"web_url":"https://patchwork.libcamera.org/comment/18075/","msgid":"","date":"2021-07-11T14:40:06","subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Jul 07, 2021 at 01:26:58PM +0100, Kieran Bingham wrote:\n> On 07/07/2021 12:46, Laurent Pinchart wrote:\n> > On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote:\n> >> On 05/07/2021 00:28, Laurent Pinchart wrote:\n> >>> Despite sharing the same name, the private data class created by the\n> >>> Extensible design pattern and the C++ private access specifier have\n> >>> different goals. The latter specifies class members private to the\n> >>> class, while the former stores data not visible to the application.\n> >>>\n> >>> There are use cases for accessing the private data class from other\n> >>> classes inside libcamera. Make this possible by exposing public _d()\n> >>> functions in the class deriving from Extensible. This won't allow access\n> >>> to the private data by applications as the definition of the Private\n> >>> class isn't visible outside of libcamera.\n> >>\n> >> It almost makes me think the name for the D ptr type should be\n> >> 'Internal', or 'Protected' rather than 'Private', as it's not private\n> >> (it can be accessed) - it's just simply not exposed ...\n> > \n> > I knew there would be a naming discussion :-)\n> > \n> > The pointer is private to the library, but not private to the class (at\n> > least not in all cases, a particular class doesn't *have* to expose its\n> > Private class if there's no need too). And it's internal to the library\n> > too, but not internal to the class.\n> > \n> >> But I'm not opposed to keeping the names we have.\n> >> Ultimately it's the same concept [private: means private to the class,\n> >> Private means private to libcamera as a whole]\n> > \n> > I should read through before answering :-)\n> > \n> >>> The _d() functions need to be defined as template functions to delay\n> >>> their evaluation, as the static_cast() operator in the Extensible::_d()\n> >>> functions needs the Private class to be fully defined.\n> >>>\n> >>> Signed-off-by: Laurent Pinchart \n> >>> ---\n> >>> include/libcamera/base/class.h | 14 ++++++++++++--\n> >>> 1 file changed, 12 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h\n> >>> index a07dac057331..8212c3d4a5ae 100644\n> >>> --- a/include/libcamera/base/class.h\n> >>> +++ b/include/libcamera/base/class.h\n> >>> @@ -33,14 +33,24 @@ namespace libcamera {\n> >>> #define LIBCAMERA_DECLARE_PRIVATE()\t\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> >>> +\tfriend class Private;\t\t\t\t\t\t\\\n> >>> +\ttemplate \t\t\t\t\t\\\n> >>\n> >> Is this some template magic (hack?), to make it a template without\n> >> actually needing to be a template or have any template parameters?\n> >>\n> >> If so it would be nice to state that in a comment so readers don't\n> >> wonder or query it.\n> > \n> > It's explained in the last paragraph of the commit message, would you\n> > like a comment here too ?\n> \n> The commit message explains that we need to use templates to delay\n> evaluation.\n> \n> But looks like a magic hack, and otherwise out of place.\n> \n> Reading this file without the commit message, it's really unclear why\n> there is a bool B = true, or even what it's supposed to do. (or more\n> importantly, that it's supposed to be simply ignored)\n\nI'll expand the explanation in the commit message. Would you prefer also\nhaving a comment here ? I would keep it short then.\n\n> >>> +\tconst Private *_d() const\t\t\t\t\t\\\n> >>> +\t{\t\t\t\t\t\t\t\t\\\n> >>> +\t\treturn Extensible::_d();\t\t\t\\\n> >>> +\t}\t\t\t\t\t\t\t\t\\\n> >>> +\ttemplate \t\t\t\t\t\\\n> >>> +\tPrivate *_d()\t\t\t\t\t\t\t\\\n> >>> +\t{\t\t\t\t\t\t\t\t\\\n> >>> +\t\treturn Extensible::_d();\t\t\t\\\n> >>> +\t}\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> >>> #define LIBCAMERA_D_PTR()\t\t\t\t\t\t\\\n> >>> -\t_d();\n> >>> +\t_d();\n> >>> \n> >>> #define LIBCAMERA_O_PTR()\t\t\t\t\t\t\\\n> >>> \t_o();\n> >>\n> >> Presumably there wouldn't ever be the need for the equivalent _o() as\n> >> that will only every be used from internally in the Private class.\n> > \n> > Correct.","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 0A906BD794\n\tfor ;\n\tSun, 11 Jul 2021 14:40:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B82556851D;\n\tSun, 11 Jul 2021 16:40:54 +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 53F1568519\n\tfor ;\n\tSun, 11 Jul 2021 16:40:53 +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 DDA85CC;\n\tSun, 11 Jul 2021 16:40:52 +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=\"vUvgLMS4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626014453;\n\tbh=A2JKjSvbUAgfn8vKALwlbHjR3iL6dgwiVetr0KS/IkM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vUvgLMS4eNfY4wUhEcLdYDkWs5GDyCmm/SdPYQixk5yAe4/VpEeFtSwy2B3QIUjAB\n\tCrcaagzmvCkP3Tzu6KZZwGK+THVG5ih2wNPmRsV7GCR+c2MuM2gVXC0xeUvxNB6X5s\n\tZfMJMLMyvrOBlgalYmh5FVXTXup7VZAMU3qa0h/M=","Date":"Sun, 11 Jul 2021 17:40:06 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210704232817.8205-1-laurent.pinchart@ideasonboard.com>\n\t<20210704232817.8205-2-laurent.pinchart@ideasonboard.com>\n\t<367fb874-a778-6cb5-8efa-2d3d1bfc5d8f@ideasonboard.com>\n\t\n\t<76564e10-de64-9ab1-ffc1-99e767341fb2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<76564e10-de64-9ab1-ffc1-99e767341fb2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/3] libcamera: base: class: Expose\n\tExtensible private data to other classes","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\" "}}]