[libcamera-devel,v2,1/2] libcamera: Drop argument from LIBCAMERA_DECLARE_PRIVATE
diff mbox series

Message ID 20210420093859.14280-2-jacopo@jmondi.org
State Accepted
Delegated to: Jacopo Mondi
Headers show
Series
  • Minor fixes to src/libcamera/class.cpp
Related show

Commit Message

Jacopo Mondi April 20, 2021, 9:38 a.m. UTC 
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.

Reviewed-by: Hanlin Chen <hanlinchen@google.com>
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 +-
 src/libcamera/class.cpp            | 4 +---
 5 files changed, 6 insertions(+), 8 deletions(-)

--
2.31.1

Comments

Niklas Söderlund April 20, 2021, 11:29 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2021-04-20 11:38:58 +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.
> 
> Reviewed-by: Hanlin Chen <hanlinchen@google.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/camera.h         | 2 +-
>  include/libcamera/camera_manager.h | 2 +-
>  include/libcamera/class.h          | 4 ++--
>  src/android/camera_buffer.h        | 2 +-
>  src/libcamera/class.cpp            | 4 +---
>  5 files changed, 6 insertions(+), 8 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);
> diff --git a/src/libcamera/class.cpp b/src/libcamera/class.cpp
> index 340b7de7911c..171f7c0a927b 100644
> --- a/src/libcamera/class.cpp
> +++ b/src/libcamera/class.cpp
> @@ -77,12 +77,10 @@ namespace libcamera {
>  /**
>   * \def LIBCAMERA_DECLARE_PRIVATE
>   * \brief Declare private data for a public class
> - * \param klass The public class name
>   *
>   * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to
>   * make a class manage its private data through a d-pointer. It shall be used at
> - * the very top of the class definition, with the public class name passed as
> - * the \a klass parameter.
> + * the very top of the class definition.
>   */
> 
>  /**
> --
> 2.31.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 20, 2021, 10:51 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Tue, Apr 20, 2021 at 11:38:58AM +0200, Jacopo Mondi wrote:
> The LIBCAMERA_DECLARE_PRIVATE() macro, used by the library classes
> that inherits from libcamera::Extensible in order to implement the

s/inherits/inherit/

> 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.
> 
> Reviewed-by: Hanlin Chen <hanlinchen@google.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/libcamera/camera.h         | 2 +-
>  include/libcamera/camera_manager.h | 2 +-
>  include/libcamera/class.h          | 4 ++--
>  src/android/camera_buffer.h        | 2 +-
>  src/libcamera/class.cpp            | 4 +---
>  5 files changed, 6 insertions(+), 8 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);
> diff --git a/src/libcamera/class.cpp b/src/libcamera/class.cpp
> index 340b7de7911c..171f7c0a927b 100644
> --- a/src/libcamera/class.cpp
> +++ b/src/libcamera/class.cpp
> @@ -77,12 +77,10 @@ namespace libcamera {
>  /**
>   * \def LIBCAMERA_DECLARE_PRIVATE
>   * \brief Declare private data for a public class
> - * \param klass The public class name
>   *
>   * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to
>   * make a class manage its private data through a d-pointer. It shall be used at
> - * the very top of the class definition, with the public class name passed as
> - * the \a klass parameter.
> + * the very top of the class definition.
>   */
> 
>  /**

Patch
diff mbox series 

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);
diff --git a/src/libcamera/class.cpp b/src/libcamera/class.cpp
index 340b7de7911c..171f7c0a927b 100644
--- a/src/libcamera/class.cpp
+++ b/src/libcamera/class.cpp
@@ -77,12 +77,10 @@  namespace libcamera {
 /**
  * \def LIBCAMERA_DECLARE_PRIVATE
  * \brief Declare private data for a public class
- * \param klass The public class name
  *
  * The LIBCAMERA_DECLARE_PRIVATE() macro plumbs the infrastructure necessary to
  * make a class manage its private data through a d-pointer. It shall be used at
- * the very top of the class definition, with the public class name passed as
- * the \a klass parameter.
+ * the very top of the class definition.
  */

 /**