| Message ID | 20201022135605.614240-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Oct 22, 2020 at 02:56:03PM +0100, Kieran Bingham wrote: > Convert MediaLink, MediaPad, and MediaEntity to declare DELETE_COPY_AND_ASSIGN. > These classes already deleted their copy constructor but not the assignment operator. > > Utilising the DELETE_COPY_AND_ASSIGN prevents all copying of these classes. Line wrap ? > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/media_object.h | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h > index be6fb8961349..f467883072d2 100644 > --- a/include/libcamera/internal/media_object.h > +++ b/include/libcamera/internal/media_object.h > @@ -12,6 +12,8 @@ > > #include <linux/media.h> > > +#include <libcamera/class.h> > + > namespace libcamera { > > class MediaDevice; > @@ -50,7 +52,7 @@ private: > > MediaLink(const struct media_v2_link *link, > MediaPad *source, MediaPad *sink); > - MediaLink(const MediaLink &) = delete; > + DELETE_COPY_AND_ASSIGN(MediaLink); You have placed DELETE_COPY_AND_ASSIGN() right after private: in patch 2/5. We could try and use the same placement in all classes for consistency. I don't mind much. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > ~MediaLink() {} > > MediaPad *source_; > @@ -72,7 +74,7 @@ private: > friend class MediaDevice; > > MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > - MediaPad(const MediaPad &) = delete; > + DELETE_COPY_AND_ASSIGN(MediaPad); > ~MediaPad(); > > unsigned int index_; > @@ -104,7 +106,7 @@ private: > > MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, > unsigned int major = 0, unsigned int minor = 0); > - MediaEntity(const MediaEntity &) = delete; > + DELETE_COPY_AND_ASSIGN(MediaEntity); > ~MediaEntity(); > > void addPad(MediaPad *pad);
On Fri, Oct 23, 2020 at 07:29:14AM +0300, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Oct 22, 2020 at 02:56:03PM +0100, Kieran Bingham wrote: > > Convert MediaLink, MediaPad, and MediaEntity to declare DELETE_COPY_AND_ASSIGN. > > These classes already deleted their copy constructor but not the assignment operator. > > > > Utilising the DELETE_COPY_AND_ASSIGN prevents all copying of these classes. > > Line wrap ? > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/internal/media_object.h | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h > > index be6fb8961349..f467883072d2 100644 > > --- a/include/libcamera/internal/media_object.h > > +++ b/include/libcamera/internal/media_object.h > > @@ -12,6 +12,8 @@ > > > > #include <linux/media.h> > > > > +#include <libcamera/class.h> > > + > > namespace libcamera { > > > > class MediaDevice; > > @@ -50,7 +52,7 @@ private: > > > > MediaLink(const struct media_v2_link *link, > > MediaPad *source, MediaPad *sink); > > - MediaLink(const MediaLink &) = delete; > > + DELETE_COPY_AND_ASSIGN(MediaLink); > > You have placed DELETE_COPY_AND_ASSIGN() right after private: in patch > 2/5. We could try and use the same placement in all classes for > consistency. I don't mind much. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Actually I think we should disable both copy and move operations, there's no need to move these classes. This is known as the rules of five: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all > > ~MediaLink() {} > > > > MediaPad *source_; > > @@ -72,7 +74,7 @@ private: > > friend class MediaDevice; > > > > MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > > - MediaPad(const MediaPad &) = delete; > > + DELETE_COPY_AND_ASSIGN(MediaPad); > > ~MediaPad(); > > > > unsigned int index_; > > @@ -104,7 +106,7 @@ private: > > > > MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, > > unsigned int major = 0, unsigned int minor = 0); > > - MediaEntity(const MediaEntity &) = delete; > > + DELETE_COPY_AND_ASSIGN(MediaEntity); > > ~MediaEntity(); > > > > void addPad(MediaPad *pad);
Hi Laurent, On 23/10/2020 05:33, Laurent Pinchart wrote: > On Fri, Oct 23, 2020 at 07:29:14AM +0300, Laurent Pinchart wrote: >> Hi Kieran, >> >> Thank you for the patch. >> >> On Thu, Oct 22, 2020 at 02:56:03PM +0100, Kieran Bingham wrote: >>> Convert MediaLink, MediaPad, and MediaEntity to declare DELETE_COPY_AND_ASSIGN. >>> These classes already deleted their copy constructor but not the assignment operator. >>> >>> Utilising the DELETE_COPY_AND_ASSIGN prevents all copying of these classes. >> >> Line wrap ? >> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> include/libcamera/internal/media_object.h | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h >>> index be6fb8961349..f467883072d2 100644 >>> --- a/include/libcamera/internal/media_object.h >>> +++ b/include/libcamera/internal/media_object.h >>> @@ -12,6 +12,8 @@ >>> >>> #include <linux/media.h> >>> >>> +#include <libcamera/class.h> >>> + >>> namespace libcamera { >>> >>> class MediaDevice; >>> @@ -50,7 +52,7 @@ private: >>> >>> MediaLink(const struct media_v2_link *link, >>> MediaPad *source, MediaPad *sink); >>> - MediaLink(const MediaLink &) = delete; >>> + DELETE_COPY_AND_ASSIGN(MediaLink); >> >> You have placed DELETE_COPY_AND_ASSIGN() right after private: in patch >> 2/5. We could try and use the same placement in all classes for >> consistency. I don't mind much. I think in those cases, there were no constructors also declared private. But I don't mind keeping the DELETE_... consistent. >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Actually I think we should disable both copy and move operations, > there's no need to move these classes. This is known as the rules of > five: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all I was trying not to change code semantics in this series, though this patch is a bit of an exception, because it was only deleting the copy constructor, without the copy assignment operator, and I considered that a specific bug. Otherwise, by that rule, we don't need DELETE_COPY_AND_ASSIGN, and we can always use DELETE_COPY_AND_MOVE in all of the other cases too. But that's most certainly more of a functional change, so I'd like to do so on top, as it will require considering each class independently. >>> ~MediaLink() {} >>> >>> MediaPad *source_; >>> @@ -72,7 +74,7 @@ private: >>> friend class MediaDevice; >>> >>> MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); >>> - MediaPad(const MediaPad &) = delete; >>> + DELETE_COPY_AND_ASSIGN(MediaPad); >>> ~MediaPad(); >>> >>> unsigned int index_; >>> @@ -104,7 +106,7 @@ private: >>> >>> MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, >>> unsigned int major = 0, unsigned int minor = 0); >>> - MediaEntity(const MediaEntity &) = delete; >>> + DELETE_COPY_AND_ASSIGN(MediaEntity); >>> ~MediaEntity(); >>> >>> void addPad(MediaPad *pad); >
Hi Kieran, On Fri, Oct 23, 2020 at 09:27:10AM +0100, Kieran Bingham wrote: > On 23/10/2020 05:33, Laurent Pinchart wrote: > > On Fri, Oct 23, 2020 at 07:29:14AM +0300, Laurent Pinchart wrote: > >> On Thu, Oct 22, 2020 at 02:56:03PM +0100, Kieran Bingham wrote: > >>> Convert MediaLink, MediaPad, and MediaEntity to declare DELETE_COPY_AND_ASSIGN. > >>> These classes already deleted their copy constructor but not the assignment operator. > >>> > >>> Utilising the DELETE_COPY_AND_ASSIGN prevents all copying of these classes. > >> > >> Line wrap ? > >> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> include/libcamera/internal/media_object.h | 8 +++++--- > >>> 1 file changed, 5 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h > >>> index be6fb8961349..f467883072d2 100644 > >>> --- a/include/libcamera/internal/media_object.h > >>> +++ b/include/libcamera/internal/media_object.h > >>> @@ -12,6 +12,8 @@ > >>> > >>> #include <linux/media.h> > >>> > >>> +#include <libcamera/class.h> > >>> + > >>> namespace libcamera { > >>> > >>> class MediaDevice; > >>> @@ -50,7 +52,7 @@ private: > >>> > >>> MediaLink(const struct media_v2_link *link, > >>> MediaPad *source, MediaPad *sink); > >>> - MediaLink(const MediaLink &) = delete; > >>> + DELETE_COPY_AND_ASSIGN(MediaLink); > >> > >> You have placed DELETE_COPY_AND_ASSIGN() right after private: in patch > >> 2/5. We could try and use the same placement in all classes for > >> consistency. I don't mind much. > > I think in those cases, there were no constructors also declared private. > > But I don't mind keeping the DELETE_... consistent. > > >> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Actually I think we should disable both copy and move operations, > > there's no need to move these classes. This is known as the rules of > > five: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all > > I was trying not to change code semantics in this series, though this > patch is a bit of an exception, because it was only deleting the copy > constructor, without the copy assignment operator, and I considered that > a specific bug. > > Otherwise, by that rule, we don't need DELETE_COPY_AND_ASSIGN, and we > can always use DELETE_COPY_AND_MOVE in all of the other cases too. Note that the rule states that all constructors and operators should be declared, not that they all have to be deleted or all explicitly implemented (unless I misunderstand something). I expect the most common case to be disabling both copy and move, but there will be some cases where copy will be disabled and move explicitly defined (std::unique_ptr<> being the best example for that, it manages a unique resource, thus disabling copying, but allows moving). > But that's most certainly more of a functional change, so I'd like to do > so on top, as it will require considering each class independently. I'm fine doing so on top. This series is a good candidate though, as if we delay it too much we'll forget about it :-) > >>> ~MediaLink() {} > >>> > >>> MediaPad *source_; > >>> @@ -72,7 +74,7 @@ private: > >>> friend class MediaDevice; > >>> > >>> MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > >>> - MediaPad(const MediaPad &) = delete; > >>> + DELETE_COPY_AND_ASSIGN(MediaPad); > >>> ~MediaPad(); > >>> > >>> unsigned int index_; > >>> @@ -104,7 +106,7 @@ private: > >>> > >>> MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, > >>> unsigned int major = 0, unsigned int minor = 0); > >>> - MediaEntity(const MediaEntity &) = delete; > >>> + DELETE_COPY_AND_ASSIGN(MediaEntity); > >>> ~MediaEntity(); > >>> > >>> void addPad(MediaPad *pad);
diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h index be6fb8961349..f467883072d2 100644 --- a/include/libcamera/internal/media_object.h +++ b/include/libcamera/internal/media_object.h @@ -12,6 +12,8 @@ #include <linux/media.h> +#include <libcamera/class.h> + namespace libcamera { class MediaDevice; @@ -50,7 +52,7 @@ private: MediaLink(const struct media_v2_link *link, MediaPad *source, MediaPad *sink); - MediaLink(const MediaLink &) = delete; + DELETE_COPY_AND_ASSIGN(MediaLink); ~MediaLink() {} MediaPad *source_; @@ -72,7 +74,7 @@ private: friend class MediaDevice; MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); - MediaPad(const MediaPad &) = delete; + DELETE_COPY_AND_ASSIGN(MediaPad); ~MediaPad(); unsigned int index_; @@ -104,7 +106,7 @@ private: MediaEntity(MediaDevice *dev, const struct media_v2_entity *entity, unsigned int major = 0, unsigned int minor = 0); - MediaEntity(const MediaEntity &) = delete; + DELETE_COPY_AND_ASSIGN(MediaEntity); ~MediaEntity(); void addPad(MediaPad *pad);
Convert MediaLink, MediaPad, and MediaEntity to declare DELETE_COPY_AND_ASSIGN. These classes already deleted their copy constructor but not the assignment operator. Utilising the DELETE_COPY_AND_ASSIGN prevents all copying of these classes. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/internal/media_object.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)