[libcamera-devel] [PATCH] libcamera: Drop argument from LIBCAMERA_DECLARE_PRIVATE

[Jacopo Mondi]

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 at 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