| Message ID | 20210704232817.8205-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Jul 05, 2021 at 02:28:15AM +0300, Laurent Pinchart wrote: > Despite sharing the same name, the private data class created by the > Extensible design pattern and the C++ private access specifier have > different goals. The latter specifies class members private to the > class, while the former stores data not visible to the application. > > There are use cases for accessing the private data class from other > classes inside libcamera. Make this possible by exposing public _d() > functions in the class deriving from Extensible. This won't allow access > to the private data by applications as the definition of the Private > class isn't visible outside of libcamera. Neat, but how can other internal class access the klass::Private implementation which is defined in the .cpp file only ? > > The _d() functions need to be defined as template functions to delay > their evaluation, as the static_cast() operator in the Extensible::_d() > functions needs the Private class to be fully defined. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/class.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > index a07dac057331..8212c3d4a5ae 100644 > --- a/include/libcamera/base/class.h > +++ b/include/libcamera/base/class.h > @@ -33,14 +33,24 @@ namespace libcamera { > #define LIBCAMERA_DECLARE_PRIVATE() \ > public: \ > class Private; \ > - friend class Private; > + friend class Private; \ > + template <bool B = true> \ > + const Private *_d() const \ > + { \ > + return Extensible::_d<Private>(); \ > + } \ > + template <bool B = true> \ > + Private *_d() \ > + { \ > + return Extensible::_d<Private>(); \ > + } > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > friend class klass; \ > using Public = klass; > > #define LIBCAMERA_D_PTR() \ > - _d<Private>(); > + _d(); > > #define LIBCAMERA_O_PTR() \ > _o<Public>(); > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Mon, Jul 05, 2021 at 12:29:34PM +0200, Jacopo Mondi wrote: > On Mon, Jul 05, 2021 at 02:28:15AM +0300, Laurent Pinchart wrote: > > Despite sharing the same name, the private data class created by the > > Extensible design pattern and the C++ private access specifier have > > different goals. The latter specifies class members private to the > > class, while the former stores data not visible to the application. > > > > There are use cases for accessing the private data class from other > > classes inside libcamera. Make this possible by exposing public _d() > > functions in the class deriving from Extensible. This won't allow access > > to the private data by applications as the definition of the Private > > class isn't visible outside of libcamera. > > Neat, but how can other internal class access the klass::Private > implementation which is defined in the .cpp file only ? Please see 3/3 :-) > > The _d() functions need to be defined as template functions to delay > > their evaluation, as the static_cast() operator in the Extensible::_d() > > functions needs the Private class to be fully defined. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/class.h | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > index a07dac057331..8212c3d4a5ae 100644 > > --- a/include/libcamera/base/class.h > > +++ b/include/libcamera/base/class.h > > @@ -33,14 +33,24 @@ namespace libcamera { > > #define LIBCAMERA_DECLARE_PRIVATE() \ > > public: \ > > class Private; \ > > - friend class Private; > > + friend class Private; \ > > + template <bool B = true> \ > > + const Private *_d() const \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } \ > > + template <bool B = true> \ > > + Private *_d() \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } > > > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > > friend class klass; \ > > using Public = klass; > > > > #define LIBCAMERA_D_PTR() \ > > - _d<Private>(); > > + _d(); > > > > #define LIBCAMERA_O_PTR() \ > > _o<Public>();
Hi Laurent, On 7/5/21 4:58 AM, Laurent Pinchart wrote: > Despite sharing the same name, the private data class created by the > Extensible design pattern and the C++ private access specifier have > different goals. The latter specifies class members private to the > class, while the former stores data not visible to the application. > > There are use cases for accessing the private data class from other > classes inside libcamera. Make this possible by exposing public _d() > functions in the class deriving from Extensible. This won't allow access > to the private data by applications as the definition of the Private > class isn't visible outside of libcamera. I like this idea. > The _d() functions need to be defined as template functions to delay > their evaluation, as the static_cast() operator in the Extensible::_d() > functions needs the Private class to be fully defined. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/base/class.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > index a07dac057331..8212c3d4a5ae 100644 > --- a/include/libcamera/base/class.h > +++ b/include/libcamera/base/class.h > @@ -33,14 +33,24 @@ namespace libcamera { > #define LIBCAMERA_DECLARE_PRIVATE() \ > public: \ > class Private; \ > - friend class Private; > + friend class Private; \ > + template <bool B = true> \ > + const Private *_d() const \ > + { \ > + return Extensible::_d<Private>(); \ > + } \ > + template <bool B = true> \ > + Private *_d() \ > + { \ > + return Extensible::_d<Private>(); \ > + } > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > friend class klass; \ > using Public = klass; > > #define LIBCAMERA_D_PTR() \ > - _d<Private>(); > + _d(); > > #define LIBCAMERA_O_PTR() \ > _o<Public>();
Hi Laurent, thank you for the patch. On Tue, Jul 6, 2021 at 2:08 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Laurent, > > On 7/5/21 4:58 AM, Laurent Pinchart wrote: > > Despite sharing the same name, the private data class created by the > > Extensible design pattern and the C++ private access specifier have > > different goals. The latter specifies class members private to the > > class, while the former stores data not visible to the application. > > > > There are use cases for accessing the private data class from other > > classes inside libcamera. Make this possible by exposing public _d() > > functions in the class deriving from Extensible. This won't allow access > > to the private data by applications as the definition of the Private > > class isn't visible outside of libcamera. > I like this idea. > > The _d() functions need to be defined as template functions to delay > > their evaluation, as the static_cast() operator in the Extensible::_d() > > functions needs the Private class to be fully defined. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/base/class.h | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > index a07dac057331..8212c3d4a5ae 100644 > > --- a/include/libcamera/base/class.h > > +++ b/include/libcamera/base/class.h > > @@ -33,14 +33,24 @@ namespace libcamera { > > #define LIBCAMERA_DECLARE_PRIVATE() \ > > public: \ > > class Private; \ > > - friend class Private; > > + friend class Private; \ > > + template <bool B = true> \ > > + const Private *_d() const \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } \ > > + template <bool B = true> \ > > + Private *_d() \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } > > > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > > friend class klass; \ > > using Public = klass; > > > > #define LIBCAMERA_D_PTR() \ > > - _d<Private>(); > > + _d(); > > > > #define LIBCAMERA_O_PTR() \ > > _o<Public>();
Hi Laurent, On 05/07/2021 00:28, Laurent Pinchart wrote: > Despite sharing the same name, the private data class created by the > Extensible design pattern and the C++ private access specifier have > different goals. The latter specifies class members private to the > class, while the former stores data not visible to the application. > > There are use cases for accessing the private data class from other > classes inside libcamera. Make this possible by exposing public _d() > functions in the class deriving from Extensible. This won't allow access > to the private data by applications as the definition of the Private > class isn't visible outside of libcamera. It almost makes me think the name for the D ptr type should be 'Internal', or 'Protected' rather than 'Private', as it's not private (it can be accessed) - it's just simply not exposed ... But I'm not opposed to keeping the names we have. Ultimately it's the same concept [private: means private to the class, Private means private to libcamera as a whole] > The _d() functions need to be defined as template functions to delay > their evaluation, as the static_cast() operator in the Extensible::_d() > functions needs the Private class to be fully defined. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/class.h | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > index a07dac057331..8212c3d4a5ae 100644 > --- a/include/libcamera/base/class.h > +++ b/include/libcamera/base/class.h > @@ -33,14 +33,24 @@ namespace libcamera { > #define LIBCAMERA_DECLARE_PRIVATE() \ > public: \ > class Private; \ > - friend class Private; > + friend class Private; \ > + template <bool B = true> \ Is this some template magic (hack?), to make it a template without actually needing to be a template or have any template parameters? If so it would be nice to state that in a comment so readers don't wonder or query it. > + const Private *_d() const \ > + { \ > + return Extensible::_d<Private>(); \ > + } \ > + template <bool B = true> \ > + Private *_d() \ > + { \ > + return Extensible::_d<Private>(); \ > + } > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > friend class klass; \ > using Public = klass; > > #define LIBCAMERA_D_PTR() \ > - _d<Private>(); > + _d(); > > #define LIBCAMERA_O_PTR() \ > _o<Public>(); > Presumably there wouldn't ever be the need for the equivalent _o() as that will only every be used from internally in the Private class. -- Kieran
Hi Kieran, On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote: > On 05/07/2021 00:28, Laurent Pinchart wrote: > > Despite sharing the same name, the private data class created by the > > Extensible design pattern and the C++ private access specifier have > > different goals. The latter specifies class members private to the > > class, while the former stores data not visible to the application. > > > > There are use cases for accessing the private data class from other > > classes inside libcamera. Make this possible by exposing public _d() > > functions in the class deriving from Extensible. This won't allow access > > to the private data by applications as the definition of the Private > > class isn't visible outside of libcamera. > > It almost makes me think the name for the D ptr type should be > 'Internal', or 'Protected' rather than 'Private', as it's not private > (it can be accessed) - it's just simply not exposed ... I knew there would be a naming discussion :-) The pointer is private to the library, but not private to the class (at least not in all cases, a particular class doesn't *have* to expose its Private class if there's no need too). And it's internal to the library too, but not internal to the class. > But I'm not opposed to keeping the names we have. > Ultimately it's the same concept [private: means private to the class, > Private means private to libcamera as a whole] I should read through before answering :-) > > The _d() functions need to be defined as template functions to delay > > their evaluation, as the static_cast() operator in the Extensible::_d() > > functions needs the Private class to be fully defined. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/class.h | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > > index a07dac057331..8212c3d4a5ae 100644 > > --- a/include/libcamera/base/class.h > > +++ b/include/libcamera/base/class.h > > @@ -33,14 +33,24 @@ namespace libcamera { > > #define LIBCAMERA_DECLARE_PRIVATE() \ > > public: \ > > class Private; \ > > - friend class Private; > > + friend class Private; \ > > + template <bool B = true> \ > > Is this some template magic (hack?), to make it a template without > actually needing to be a template or have any template parameters? > > If so it would be nice to state that in a comment so readers don't > wonder or query it. It's explained in the last paragraph of the commit message, would you like a comment here too ? > > + const Private *_d() const \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } \ > > + template <bool B = true> \ > > + Private *_d() \ > > + { \ > > + return Extensible::_d<Private>(); \ > > + } > > > > #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > > friend class klass; \ > > using Public = klass; > > > > #define LIBCAMERA_D_PTR() \ > > - _d<Private>(); > > + _d(); > > > > #define LIBCAMERA_O_PTR() \ > > _o<Public>(); > > Presumably there wouldn't ever be the need for the equivalent _o() as > that will only every be used from internally in the Private class. Correct.
Hi Laurent, On 07/07/2021 12:46, Laurent Pinchart wrote: > Hi Kieran, > > On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote: >> On 05/07/2021 00:28, Laurent Pinchart wrote: >>> Despite sharing the same name, the private data class created by the >>> Extensible design pattern and the C++ private access specifier have >>> different goals. The latter specifies class members private to the >>> class, while the former stores data not visible to the application. >>> >>> There are use cases for accessing the private data class from other >>> classes inside libcamera. Make this possible by exposing public _d() >>> functions in the class deriving from Extensible. This won't allow access >>> to the private data by applications as the definition of the Private >>> class isn't visible outside of libcamera. >> >> It almost makes me think the name for the D ptr type should be >> 'Internal', or 'Protected' rather than 'Private', as it's not private >> (it can be accessed) - it's just simply not exposed ... > > I knew there would be a naming discussion :-) > > The pointer is private to the library, but not private to the class (at > least not in all cases, a particular class doesn't *have* to expose its > Private class if there's no need too). And it's internal to the library > too, but not internal to the class. > >> But I'm not opposed to keeping the names we have. >> Ultimately it's the same concept [private: means private to the class, >> Private means private to libcamera as a whole] > > I should read through before answering :-) > >>> The _d() functions need to be defined as template functions to delay >>> their evaluation, as the static_cast() operator in the Extensible::_d() >>> functions needs the Private class to be fully defined. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> include/libcamera/base/class.h | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h >>> index a07dac057331..8212c3d4a5ae 100644 >>> --- a/include/libcamera/base/class.h >>> +++ b/include/libcamera/base/class.h >>> @@ -33,14 +33,24 @@ namespace libcamera { >>> #define LIBCAMERA_DECLARE_PRIVATE() \ >>> public: \ >>> class Private; \ >>> - friend class Private; >>> + friend class Private; \ >>> + template <bool B = true> \ >> >> Is this some template magic (hack?), to make it a template without >> actually needing to be a template or have any template parameters? >> >> If so it would be nice to state that in a comment so readers don't >> wonder or query it. > > It's explained in the last paragraph of the commit message, would you > like a comment here too ? The commit message explains that we need to use templates to delay evaluation. But <bool B = true> looks like a magic hack, and otherwise out of place. Reading this file without the commit message, it's really unclear why there is a bool B = true, or even what it's supposed to do. (or more importantly, that it's supposed to be simply ignored) >>> + const Private *_d() const \ >>> + { \ >>> + return Extensible::_d<Private>(); \ >>> + } \ >>> + template <bool B = true> \ >>> + Private *_d() \ >>> + { \ >>> + return Extensible::_d<Private>(); \ >>> + } >>> >>> #define LIBCAMERA_DECLARE_PUBLIC(klass) \ >>> friend class klass; \ >>> using Public = klass; >>> >>> #define LIBCAMERA_D_PTR() \ >>> - _d<Private>(); >>> + _d(); >>> >>> #define LIBCAMERA_O_PTR() \ >>> _o<Public>(); >> >> Presumably there wouldn't ever be the need for the equivalent _o() as >> that will only every be used from internally in the Private class. > > Correct. >
Hi Kieran, On Wed, Jul 07, 2021 at 01:26:58PM +0100, Kieran Bingham wrote: > On 07/07/2021 12:46, Laurent Pinchart wrote: > > On Wed, Jul 07, 2021 at 09:55:18AM +0100, Kieran Bingham wrote: > >> On 05/07/2021 00:28, Laurent Pinchart wrote: > >>> Despite sharing the same name, the private data class created by the > >>> Extensible design pattern and the C++ private access specifier have > >>> different goals. The latter specifies class members private to the > >>> class, while the former stores data not visible to the application. > >>> > >>> There are use cases for accessing the private data class from other > >>> classes inside libcamera. Make this possible by exposing public _d() > >>> functions in the class deriving from Extensible. This won't allow access > >>> to the private data by applications as the definition of the Private > >>> class isn't visible outside of libcamera. > >> > >> It almost makes me think the name for the D ptr type should be > >> 'Internal', or 'Protected' rather than 'Private', as it's not private > >> (it can be accessed) - it's just simply not exposed ... > > > > I knew there would be a naming discussion :-) > > > > The pointer is private to the library, but not private to the class (at > > least not in all cases, a particular class doesn't *have* to expose its > > Private class if there's no need too). And it's internal to the library > > too, but not internal to the class. > > > >> But I'm not opposed to keeping the names we have. > >> Ultimately it's the same concept [private: means private to the class, > >> Private means private to libcamera as a whole] > > > > I should read through before answering :-) > > > >>> The _d() functions need to be defined as template functions to delay > >>> their evaluation, as the static_cast() operator in the Extensible::_d() > >>> functions needs the Private class to be fully defined. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> include/libcamera/base/class.h | 14 ++++++++++++-- > >>> 1 file changed, 12 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h > >>> index a07dac057331..8212c3d4a5ae 100644 > >>> --- a/include/libcamera/base/class.h > >>> +++ b/include/libcamera/base/class.h > >>> @@ -33,14 +33,24 @@ namespace libcamera { > >>> #define LIBCAMERA_DECLARE_PRIVATE() \ > >>> public: \ > >>> class Private; \ > >>> - friend class Private; > >>> + friend class Private; \ > >>> + template <bool B = true> \ > >> > >> Is this some template magic (hack?), to make it a template without > >> actually needing to be a template or have any template parameters? > >> > >> If so it would be nice to state that in a comment so readers don't > >> wonder or query it. > > > > It's explained in the last paragraph of the commit message, would you > > like a comment here too ? > > The commit message explains that we need to use templates to delay > evaluation. > > But <bool B = true> looks like a magic hack, and otherwise out of place. > > Reading this file without the commit message, it's really unclear why > there is a bool B = true, or even what it's supposed to do. (or more > importantly, that it's supposed to be simply ignored) I'll expand the explanation in the commit message. Would you prefer also having a comment here ? I would keep it short then. > >>> + const Private *_d() const \ > >>> + { \ > >>> + return Extensible::_d<Private>(); \ > >>> + } \ > >>> + template <bool B = true> \ > >>> + Private *_d() \ > >>> + { \ > >>> + return Extensible::_d<Private>(); \ > >>> + } > >>> > >>> #define LIBCAMERA_DECLARE_PUBLIC(klass) \ > >>> friend class klass; \ > >>> using Public = klass; > >>> > >>> #define LIBCAMERA_D_PTR() \ > >>> - _d<Private>(); > >>> + _d(); > >>> > >>> #define LIBCAMERA_O_PTR() \ > >>> _o<Public>(); > >> > >> Presumably there wouldn't ever be the need for the equivalent _o() as > >> that will only every be used from internally in the Private class. > > > > Correct.
diff --git a/include/libcamera/base/class.h b/include/libcamera/base/class.h index a07dac057331..8212c3d4a5ae 100644 --- a/include/libcamera/base/class.h +++ b/include/libcamera/base/class.h @@ -33,14 +33,24 @@ namespace libcamera { #define LIBCAMERA_DECLARE_PRIVATE() \ public: \ class Private; \ - friend class Private; + friend class Private; \ + template <bool B = true> \ + const Private *_d() const \ + { \ + return Extensible::_d<Private>(); \ + } \ + template <bool B = true> \ + Private *_d() \ + { \ + return Extensible::_d<Private>(); \ + } #define LIBCAMERA_DECLARE_PUBLIC(klass) \ friend class klass; \ using Public = klass; #define LIBCAMERA_D_PTR() \ - _d<Private>(); + _d(); #define LIBCAMERA_O_PTR() \ _o<Public>();
Despite sharing the same name, the private data class created by the Extensible design pattern and the C++ private access specifier have different goals. The latter specifies class members private to the class, while the former stores data not visible to the application. There are use cases for accessing the private data class from other classes inside libcamera. Make this possible by exposing public _d() functions in the class deriving from Extensible. This won't allow access to the private data by applications as the definition of the Private class isn't visible outside of libcamera. The _d() functions need to be defined as template functions to delay their evaluation, as the static_cast() operator in the Extensible::_d() functions needs the Private class to be fully defined. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/class.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)