| Message ID | 20210211133444.764808-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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 ;. > + > +#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) > + > #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/ ? 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. > + */ > + > +/** > + * \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
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@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 >
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@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 > >
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 + +#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) + #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. + * \param klass The identifier of the class to modify + */ + +/** + * \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
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 | 12 ++++++++++++ src/libcamera/class.cpp | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+)