Message ID | 20210418145545.29250-1-jacopo@jmondi.org |
---|---|
State | Changes Requested |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, On Sun, Apr 18, 2021 at 10:55 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > The LIBCAMERA_DECLARE_PRIVATE() macro, used by the library classes > that inherits from libcamera::Extensible in order to implement the > PIMPL pattern, expands to: > > public: \ > class Private; \ > friend class Private; > > The 'klass' argument is not used and it might confuse developers as > it might hint that the class that defines the pattern's implementation > can be freely named, while it is actually hardcoded to 'Private'. > > Drop the argument from the macro definition. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 2 +- > include/libcamera/camera_manager.h | 2 +- > include/libcamera/class.h | 4 ++-- > src/android/camera_buffer.h | 2 +- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 326b14d0ca01..d71641805c0a 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -74,7 +74,7 @@ protected: > class Camera final : public Object, public std::enable_shared_from_this<Camera>, > public Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(Camera) > + LIBCAMERA_DECLARE_PRIVATE() > > public: > static std::shared_ptr<Camera> create(PipelineHandler *pipe, > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 35a59f0df4ca..c2f0b786da8e 100644 > --- a/include/libcamera/camera_manager.h > +++ b/include/libcamera/camera_manager.h > @@ -22,7 +22,7 @@ class Camera; > > class CameraManager : public Object, public Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(CameraManager) > + LIBCAMERA_DECLARE_PRIVATE() > public: > CameraManager(); > ~CameraManager(); > diff --git a/include/libcamera/class.h b/include/libcamera/class.h > index 920624d8e726..466114ecfaf4 100644 > --- a/include/libcamera/class.h > +++ b/include/libcamera/class.h > @@ -30,7 +30,7 @@ namespace libcamera { > #endif > > #ifndef __DOXYGEN__ > -#define LIBCAMERA_DECLARE_PRIVATE(klass) \ > +#define LIBCAMERA_DECLARE_PRIVATE() \ > public: \ > class Private; \ > friend class Private; > @@ -46,7 +46,7 @@ public: \ > _o<Public>(); > > #else > -#define LIBCAMERA_DECLARE_PRIVATE(klass) > +#define LIBCAMERA_DECLARE_PRIVATE() > #define LIBCAMERA_DECLARE_PUBLIC(klass) > #define LIBCAMERA_D_PTR(klass) > #define LIBCAMERA_O_PTR(klass) > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > index 7e8970b49f49..c88124b2b3f3 100644 > --- a/src/android/camera_buffer.h > +++ b/src/android/camera_buffer.h > @@ -14,7 +14,7 @@ > > class CameraBuffer final : public libcamera::Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) > + LIBCAMERA_DECLARE_PRIVATE() > > public: > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel Indeed "Private" is intended to be an incomplete type, and we don't have much to do with it. Thanks for the patch. Reviewed-by: Hanlin Chen <hanlinchen@google.com> -- Cheers. Hanlin Chen
Hi Jacopo, Thank you for the patch. On Sun, Apr 18, 2021 at 04:55:45PM +0200, Jacopo Mondi wrote: > The LIBCAMERA_DECLARE_PRIVATE() macro, used by the library classes > that inherits from libcamera::Extensible in order to implement the > PIMPL pattern, expands to: > > public: \ > class Private; \ > friend class Private; > > The 'klass' argument is not used and it might confuse developers as > it might hint that the class that defines the pattern's implementation > can be freely named, while it is actually hardcoded to 'Private'. > > Drop the argument from the macro definition. I've intentionally kept the argument (I think this has actually been discussed before) for symmetry with LIBCAMERA_DECLARE_PUBLIC(), and perhaps, unconciously, wondering if it would be needed later. Those are pretty weak reasons, and it could indeed be confusing for developers. I don't mind much either way, but if you'd like to merge this patch, you've forgotten to update the documentation of the macro in class.cpp. Did doxygen remain silent ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/camera.h | 2 +- > include/libcamera/camera_manager.h | 2 +- > include/libcamera/class.h | 4 ++-- > src/android/camera_buffer.h | 2 +- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 326b14d0ca01..d71641805c0a 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -74,7 +74,7 @@ protected: > class Camera final : public Object, public std::enable_shared_from_this<Camera>, > public Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(Camera) > + LIBCAMERA_DECLARE_PRIVATE() > > public: > static std::shared_ptr<Camera> create(PipelineHandler *pipe, > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > index 35a59f0df4ca..c2f0b786da8e 100644 > --- a/include/libcamera/camera_manager.h > +++ b/include/libcamera/camera_manager.h > @@ -22,7 +22,7 @@ class Camera; > > class CameraManager : public Object, public Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(CameraManager) > + LIBCAMERA_DECLARE_PRIVATE() > public: > CameraManager(); > ~CameraManager(); > diff --git a/include/libcamera/class.h b/include/libcamera/class.h > index 920624d8e726..466114ecfaf4 100644 > --- a/include/libcamera/class.h > +++ b/include/libcamera/class.h > @@ -30,7 +30,7 @@ namespace libcamera { > #endif > > #ifndef __DOXYGEN__ > -#define LIBCAMERA_DECLARE_PRIVATE(klass) \ > +#define LIBCAMERA_DECLARE_PRIVATE() \ > public: \ > class Private; \ > friend class Private; > @@ -46,7 +46,7 @@ public: \ > _o<Public>(); > > #else > -#define LIBCAMERA_DECLARE_PRIVATE(klass) > +#define LIBCAMERA_DECLARE_PRIVATE() > #define LIBCAMERA_DECLARE_PUBLIC(klass) > #define LIBCAMERA_D_PTR(klass) > #define LIBCAMERA_O_PTR(klass) > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > index 7e8970b49f49..c88124b2b3f3 100644 > --- a/src/android/camera_buffer.h > +++ b/src/android/camera_buffer.h > @@ -14,7 +14,7 @@ > > class CameraBuffer final : public libcamera::Extensible > { > - LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) > + LIBCAMERA_DECLARE_PRIVATE() > > public: > CameraBuffer(buffer_handle_t camera3Buffer, int flags);
Hi Laurent, On Tue, Apr 20, 2021 at 03:41:00AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Sun, Apr 18, 2021 at 04:55:45PM +0200, Jacopo Mondi wrote: > > The LIBCAMERA_DECLARE_PRIVATE() macro, used by the library classes > > that inherits from libcamera::Extensible in order to implement the > > PIMPL pattern, expands to: > > > > public: \ > > class Private; \ > > friend class Private; > > > > The 'klass' argument is not used and it might confuse developers as > > it might hint that the class that defines the pattern's implementation > > can be freely named, while it is actually hardcoded to 'Private'. > > > > Drop the argument from the macro definition. > > I've intentionally kept the argument (I think this has actually been > discussed before) for symmetry with LIBCAMERA_DECLARE_PUBLIC(), and I might have some memories about that now that you've mentioned it, yes... > perhaps, unconciously, wondering if it would be needed later. Those are > pretty weak reasons, and it could indeed be confusing for developers. I > don't mind much either way, but if you'd like to merge this patch, > you've forgotten to update the documentation of the macro in class.cpp. > Did doxygen remain silent ? You know, it does not :/ The documentation reports * ... It shall be used at * the very top of the class definition, with the public class name passed as * the \a klass parameter. I think it's a bit confusing, yes... Thanks j > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/camera.h | 2 +- > > include/libcamera/camera_manager.h | 2 +- > > include/libcamera/class.h | 4 ++-- > > src/android/camera_buffer.h | 2 +- > > 4 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > > index 326b14d0ca01..d71641805c0a 100644 > > --- a/include/libcamera/camera.h > > +++ b/include/libcamera/camera.h > > @@ -74,7 +74,7 @@ protected: > > class Camera final : public Object, public std::enable_shared_from_this<Camera>, > > public Extensible > > { > > - LIBCAMERA_DECLARE_PRIVATE(Camera) > > + LIBCAMERA_DECLARE_PRIVATE() > > > > public: > > static std::shared_ptr<Camera> create(PipelineHandler *pipe, > > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h > > index 35a59f0df4ca..c2f0b786da8e 100644 > > --- a/include/libcamera/camera_manager.h > > +++ b/include/libcamera/camera_manager.h > > @@ -22,7 +22,7 @@ class Camera; > > > > class CameraManager : public Object, public Extensible > > { > > - LIBCAMERA_DECLARE_PRIVATE(CameraManager) > > + LIBCAMERA_DECLARE_PRIVATE() > > public: > > CameraManager(); > > ~CameraManager(); > > diff --git a/include/libcamera/class.h b/include/libcamera/class.h > > index 920624d8e726..466114ecfaf4 100644 > > --- a/include/libcamera/class.h > > +++ b/include/libcamera/class.h > > @@ -30,7 +30,7 @@ namespace libcamera { > > #endif > > > > #ifndef __DOXYGEN__ > > -#define LIBCAMERA_DECLARE_PRIVATE(klass) \ > > +#define LIBCAMERA_DECLARE_PRIVATE() \ > > public: \ > > class Private; \ > > friend class Private; > > @@ -46,7 +46,7 @@ public: \ > > _o<Public>(); > > > > #else > > -#define LIBCAMERA_DECLARE_PRIVATE(klass) > > +#define LIBCAMERA_DECLARE_PRIVATE() > > #define LIBCAMERA_DECLARE_PUBLIC(klass) > > #define LIBCAMERA_D_PTR(klass) > > #define LIBCAMERA_O_PTR(klass) > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h > > index 7e8970b49f49..c88124b2b3f3 100644 > > --- a/src/android/camera_buffer.h > > +++ b/src/android/camera_buffer.h > > @@ -14,7 +14,7 @@ > > > > class CameraBuffer final : public libcamera::Extensible > > { > > - LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) > > + LIBCAMERA_DECLARE_PRIVATE() > > > > public: > > CameraBuffer(buffer_handle_t camera3Buffer, int flags); > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index 326b14d0ca01..d71641805c0a 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -74,7 +74,7 @@ protected: class Camera final : public Object, public std::enable_shared_from_this<Camera>, public Extensible { - LIBCAMERA_DECLARE_PRIVATE(Camera) + LIBCAMERA_DECLARE_PRIVATE() public: static std::shared_ptr<Camera> create(PipelineHandler *pipe, diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h index 35a59f0df4ca..c2f0b786da8e 100644 --- a/include/libcamera/camera_manager.h +++ b/include/libcamera/camera_manager.h @@ -22,7 +22,7 @@ class Camera; class CameraManager : public Object, public Extensible { - LIBCAMERA_DECLARE_PRIVATE(CameraManager) + LIBCAMERA_DECLARE_PRIVATE() public: CameraManager(); ~CameraManager(); diff --git a/include/libcamera/class.h b/include/libcamera/class.h index 920624d8e726..466114ecfaf4 100644 --- a/include/libcamera/class.h +++ b/include/libcamera/class.h @@ -30,7 +30,7 @@ namespace libcamera { #endif #ifndef __DOXYGEN__ -#define LIBCAMERA_DECLARE_PRIVATE(klass) \ +#define LIBCAMERA_DECLARE_PRIVATE() \ public: \ class Private; \ friend class Private; @@ -46,7 +46,7 @@ public: \ _o<Public>(); #else -#define LIBCAMERA_DECLARE_PRIVATE(klass) +#define LIBCAMERA_DECLARE_PRIVATE() #define LIBCAMERA_DECLARE_PUBLIC(klass) #define LIBCAMERA_D_PTR(klass) #define LIBCAMERA_O_PTR(klass) diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h index 7e8970b49f49..c88124b2b3f3 100644 --- a/src/android/camera_buffer.h +++ b/src/android/camera_buffer.h @@ -14,7 +14,7 @@ class CameraBuffer final : public libcamera::Extensible { - LIBCAMERA_DECLARE_PRIVATE(CameraBuffer) + LIBCAMERA_DECLARE_PRIVATE() public: CameraBuffer(buffer_handle_t camera3Buffer, int flags);
The LIBCAMERA_DECLARE_PRIVATE() macro, used by the library classes that inherits from libcamera::Extensible in order to implement the PIMPL pattern, expands to: public: \ class Private; \ friend class Private; The 'klass' argument is not used and it might confuse developers as it might hint that the class that defines the pattern's implementation can be freely named, while it is actually hardcoded to 'Private'. Drop the argument from the macro definition. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/camera.h | 2 +- include/libcamera/camera_manager.h | 2 +- include/libcamera/class.h | 4 ++-- src/android/camera_buffer.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)