| Message ID | 20210212133056.873230-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 41b6d83e6a0c39b31464fce67e69f0d4c5c905cc |
| Headers | show |
| Series |
|
| Related | show |
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@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 :-) > + 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. > + > #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. Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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@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@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 >
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@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@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
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; \ + 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 + #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 + * + * 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
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@ideasonboard.com> --- include/libcamera/class.h | 18 +++++++++++++ src/libcamera/class.cpp | 57 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+)