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

[Kieran Bingham]

On 11/02/2021 21:08, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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.


>> +
>> +#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.


>> +
>>  #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.


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