[libcamera-devel] [PATCH v3 2/7] libcamera: class: Provide move and copy disablers

[Kieran Bingham]

Hi Laurent,

On 12/02/2021 14:07, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Feb 12, 2021 at 01:30:51PM +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 | 18 +++++++++++++
>>  src/libcamera/class.cpp   | 57 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/include/libcamera/class.h b/include/libcamera/class.h
>> index cb278e58204a..920624d8e726 100644
>> --- a/include/libcamera/class.h
>> +++ b/include/libcamera/class.h
>> @@ -11,6 +11,24 @@
>>  
>>  namespace libcamera {
>>  
>> +#ifndef __DOXYGEN__
>> +#define LIBCAMERA_DISABLE_COPY(klass)  \
>> +	klass(const klass &) = delete; \
> 
> Perhaps the \ could be aligned with tabs, with the same indentation for
> all macros ? We don't have a coding style rule for this, so you can pick
> what you like best :-)

Checkstyle/clang-format really seems to want to pull those in ;-)
Which implies we 'do' have a coding style already.

It seems that although we don't have
 AlignEscapedNewlines

defined in .clang-format (it's commented out), it is defaulting to 'left'.

I've also tried to set this to 'right' expecting it to match as you have
the other macros but it seems identical to left.

Personally, for anything whitespace/trivial, I'd really like the tools
to define the rules. So I'd like to go with what clang-format generates,
and that way further contributions will be consistent.





>> +	klass &operator=(const klass &) = delete;
>> +
>> +#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)
>> +#else
>> +#define LIBCAMERA_DISABLE_COPY(klass)
>> +#define LIBCAMERA_DISABLE_MOVE(klass)
>> +#define LIBCAMERA_DISABLE_COPY_AND_MOVE(klass)
>> +#endif
> 
> You could merge this with the next block to avoid two #ifndef
> __DOXYGEN__, up to you.
> 

I specifically chose this to keep them in separate groups.


>> +
>>  #ifndef __DOXYGEN__
>>  #define LIBCAMERA_DECLARE_PRIVATE(klass)				\
>>  public:									\
>> diff --git a/src/libcamera/class.cpp b/src/libcamera/class.cpp
>> index ce230be91e61..c1db14197b4e 100644
>> --- a/src/libcamera/class.cpp
>> +++ b/src/libcamera/class.cpp
>> @@ -17,6 +17,63 @@
>>  
>>  namespace libcamera {
>>  
>> +/**
>> + * \def LIBCAMERA_DISABLE_COPY
>> + * \brief Disable copy construction and assignment of the \a klass
>> + * \param klass The name of the class
>> + *
>> + * Example usage:
>> + * \code{.cpp}
>> + * class NonCopyable
>> + * {
>> + * public:
>> + * 	NonCopyable();
>> + * 	...
>> + *
>> + * private:
>> + * 	LIBCAMERA_DISABLE_COPY(NonCopyable)
>> + * };
>> + * \endcode
>> + */
>> +
>> +/**
>> + * \def LIBCAMERA_DISABLE_MOVE
>> + * \brief Disable move construction and assignment of the \a klass
>> + * \param klass The name of the class
>> + *
>> + * Example usage:
>> + * \code{.cpp}
>> + * class NonMoveable
>> + * {
>> + * public:
>> + * 	NonMoveable();
>> + * 	...
>> + *
>> + * private:
>> + * 	LIBCAMERA_DISABLE_MOVE(NonMoveable)
>> + * };
>> + * \endcode
>> + */
>> +
>> +/**
>> + * \def LIBCAMERA_DISABLE_COPY_AND_MOVE
>> + * \brief Disable copy and move construction and assignment of the \a klass
>> +* \param klass The name of the class
> 
> Missing space at the beginning of the line.

Argh. added.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> + *
>> + * Example usage:
>> + * \code{.cpp}
>> + * class NonCopyableNonMoveable
>> + * {
>> + * public:
>> + * 	NonCopyableNonMoveable();
>> + * 	...
>> + *
>> + * private:
>> + * 	LIBCAMERA_DISABLE_COPY_AND_MOVE(NonCopyableNonMoveable)
>> + * };
>> + * \endcode
>> + */
>> +
>>  /**
>>   * \def LIBCAMERA_DECLARE_PRIVATE
>>   * \brief Declare private data for a public class
> 

-- 
Regards
--
Kieran