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

[Laurent Pinchart]

Hi Kieran,

On Fri, Feb 12, 2021 at 02:34:25PM +0000, Kieran Bingham wrote:
> On 12/02/2021 14:07, Laurent Pinchart wrote:
> > 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.

That's because we don't set ColumnLimit.

There are quite a few new option for clang-format, our configuration
file needs a brush up. I'll give it a go, you can ignore the issue here
for now.

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

Laurent Pinchart