[libcamera-devel] [PATCH 3/5] libcamera: media_object: Utilise DELETE_COPY_AND_ASSIGN

[Laurent Pinchart]

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 at 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 at 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);

-- 
Regards,

Laurent Pinchart