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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran