[libcamera-devel] [PATCH v2 2/6] libcamera: class: Provide move and copy disablers

[Laurent Pinchart]

Hi Kieran,

On Fri, Feb 12, 2021 at 11:55:34AM +0000, Kieran Bingham wrote:
> On 11/02/2021 21:08, Laurent Pinchart wrote:
> > On Thu, Feb 11, 2021 at 01:34:40PM +0000, Kieran Bingham wrote:
> >> It can be difficult to correctly parse the syntax for copy/move and the
> >> associated assignment operators.
> >>
> >> Provide helpers as syntactic sugar to facilitate disabling either the
> >> copy constructor, and copy assignment operator, and the move constructor
> >> and move assignment operator in a way which is explicit and clear.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  include/libcamera/class.h | 12 ++++++++++++
> >>  src/libcamera/class.cpp   | 18 ++++++++++++++++++
> >>  2 files changed, 30 insertions(+)
> >>
> >> diff --git a/include/libcamera/class.h b/include/libcamera/class.h
> >> index 2d9b7ebfdb08..5cd31d1b9f37 100644
> >> --- a/include/libcamera/class.h
> >> +++ b/include/libcamera/class.h
> >> @@ -11,6 +11,18 @@
> >>  
> >>  namespace libcamera {
> >>  
> >> +#define LIBCAMERA_DISABLE_COPY(klass)  \
> >> +	klass(const klass &) = delete; \
> >> +	klass &operator=(const klass &) = delete
> > 
> > I'd add a ; here as other similar macros include the last ;.
> 
> lots of extra semi-colons but yes, I remember that was previously discussed.

I don't think there will be extra semicolons, the user shouldn't add
any.

> >> +
> >> +#define LIBCAMERA_DISABLE_MOVE(klass) \
> >> +	klass(klass &&) = delete;     \
> >> +	klass &operator=(klass &&) = delete
> >> +
> >> +#define LIBCAMERA_DISABLE_COPY_AND_MOVE(klass) \
> >> +	LIBCAMERA_DISABLE_COPY(klass);         \
> >> +	LIBCAMERA_DISABLE_MOVE(klass)
> 
> That's now going to add two extra semi-colons of course ;-) But there's
> no difference between one, two or more 'free' semi-colons;;;; so it
> should be fine.

Same here,

#define LIBCAMERA_DISABLE_COPY_AND_MOVE(klass)	\
	LIBCAMERA_DISABLE_COPY(klass)		\
	LIBCAMERA_DISABLE_MOVE(klass)

> >> +
> >>  #ifndef __DOXYGEN__
> >>  #define LIBCAMERA_DECLARE_PRIVATE(klass)				\
> >>  public:									\
> >> diff --git a/src/libcamera/class.cpp b/src/libcamera/class.cpp
> >> index 8a608edb369b..b081d05ec3ac 100644
> >> --- a/src/libcamera/class.cpp
> >> +++ b/src/libcamera/class.cpp
> >> @@ -17,6 +17,24 @@
> >>  
> >>  namespace libcamera {
> >>  
> >> +/**
> >> + * \def LIBCAMERA_DISABLE_COPY
> >> + * \brief Delete the copy constructor and assignment operator.
> > 
> > s/.$// and same below. I'd write "Disable copy construction and
> > assignment of the \a klass" to match the macro name.
> > 
> >> + * \param klass The identifier of the class to modify
> > 
> > s/identifier/name/
> > 
> > and s/to modify/for which to disable copy/ ?
> 
> 'for which to' sounds hideous and sparks my Grammar-Warning-Light.
> Anything else I can think of duplicates the brief and becomes really long.
> 
> Other uses of klass throughout this file simply call it
>   "The public class name"
> 
> Given the brief is already descriptive, I don't think we need to
> duplicate the actions.

OK.

> > I'd also add example usage :
> > 
> >  * To disable copy of a class, use the LIBCAMERA_DISABLE_COPY macro in
> >  * the private section of the of the class declaration:
> >  *
> >  * \code{.cpp}
> >  * class NonCopyable
> >  * {
> >  * public:
> >  * 	NonCopyable();
> >  * 	...
> >  *
> >  * private:
> >  * 	LIBCAMERA_DISABLE_COPY(NonCopyable)
> >  * };
> >  * \encode
> > 
> > Same below.
> 
> Added.
> 
> >> + */
> >> +
> >> +/**
> >> + * \def LIBCAMERA_DISABLE_MOVE
> >> + * \brief Delete the move construtor and assignment operator.
> >> + * \param klass The identifier of the class to modify
> >> + */
> >> +
> >> +/**
> >> + * \def LIBCAMERA_DISABLE_COPY_AND_MOVE
> >> + * \brief Delete all copy and move constructors, and assignment operators.
> >> + * \param klass The identifier of the class to modify
> >> + */
> >> +
> >>  /**
> >>   * \def LIBCAMERA_DECLARE_PRIVATE
> >>   * \brief Declare private data for a public class
> > 

-- 
Regards,

Laurent Pinchart