[libcamera-devel] FrameBufferAllocator: fix non-copyability

9828 diff mbox series

FrameBufferAllocator is supposed to delete copy constructor and
copy-assignment operator. It doesn't do that as it uses Camera as a
parameter instead of FrameBufferAllocator.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>
---
 include/libcamera/framebuffer_allocator.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi Tomi,

Thank you for the patch.

On Fri, Sep 25, 2020 at 05:05:25PM +0300, Tomi Valkeinen wrote:
> FrameBufferAllocator is supposed to delete copy constructor and
> copy-assignment operator. It doesn't do that as it uses Camera as a
> parameter instead of FrameBufferAllocator.

Oops... Good catch.

It's a shame we can't have unit tests whose compilation failure would
indicate success, to catch issues like these.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/libcamera/framebuffer_allocator.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> index 78f1353..a96aaea 100644
> --- a/include/libcamera/framebuffer_allocator.h
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -21,8 +21,8 @@ class FrameBufferAllocator
>  {
>  public:
>  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> -	FrameBufferAllocator(const Camera &) = delete;
> -	FrameBufferAllocator &operator=(const Camera &) = delete;
> +	FrameBufferAllocator(const FrameBufferAllocator &) = delete;
> +	FrameBufferAllocator &operator=(const FrameBufferAllocator &) = delete;
>  
>  	~FrameBufferAllocator();
>

Hi Tomi, Laurent,

On 25/09/2020 15:07, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Sep 25, 2020 at 05:05:25PM +0300, Tomi Valkeinen wrote:
>> FrameBufferAllocator is supposed to delete copy constructor and
>> copy-assignment operator. It doesn't do that as it uses Camera as a
>> parameter instead of FrameBufferAllocator.
> 
> Oops... Good catch.
> 
> It's a shame we can't have unit tests whose compilation failure would
> indicate success, to catch issues like these.

Should we perhaps make use of a macro to ensure this doesn't happen?
something akin to the Chromium DISALLOW_COPY_AND_ASSIGN type macros?

(Though, deleting them, rather than making them private).


>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

But for this,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
>> ---
>>  include/libcamera/framebuffer_allocator.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
>> index 78f1353..a96aaea 100644
>> --- a/include/libcamera/framebuffer_allocator.h
>> +++ b/include/libcamera/framebuffer_allocator.h
>> @@ -21,8 +21,8 @@ class FrameBufferAllocator
>>  {
>>  public:
>>  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
>> -	FrameBufferAllocator(const Camera &) = delete;
>> -	FrameBufferAllocator &operator=(const Camera &) = delete;
>> +	FrameBufferAllocator(const FrameBufferAllocator &) = delete;
>> +	FrameBufferAllocator &operator=(const FrameBufferAllocator &) = delete;




>>  
>>  	~FrameBufferAllocator();
>>  
>

On 25/09/2020 17:15, Kieran Bingham wrote:
> Hi Tomi, Laurent,
> 
> On 25/09/2020 15:07, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> Thank you for the patch.
>>
>> On Fri, Sep 25, 2020 at 05:05:25PM +0300, Tomi Valkeinen wrote:
>>> FrameBufferAllocator is supposed to delete copy constructor and
>>> copy-assignment operator. It doesn't do that as it uses Camera as a
>>> parameter instead of FrameBufferAllocator.
>>
>> Oops... Good catch.
>>
>> It's a shame we can't have unit tests whose compilation failure would
>> indicate success, to catch issues like these.
> 
> Should we perhaps make use of a macro to ensure this doesn't happen?
> something akin to the Chromium DISALLOW_COPY_AND_ASSIGN type macros?
> 
> (Though, deleting them, rather than making them private).

Probably also a base class can do this:

https://www.boost.org/doc/libs/1_65_0/libs/core/doc/html/core/noncopyable.html

 Tomi

Hi Tomi,

On Fri, Sep 25, 2020 at 05:18:36PM +0300, Tomi Valkeinen wrote:
> On 25/09/2020 17:15, Kieran Bingham wrote:
> > On 25/09/2020 15:07, Laurent Pinchart wrote:
> >> On Fri, Sep 25, 2020 at 05:05:25PM +0300, Tomi Valkeinen wrote:
> >>> FrameBufferAllocator is supposed to delete copy constructor and
> >>> copy-assignment operator. It doesn't do that as it uses Camera as a
> >>> parameter instead of FrameBufferAllocator.
> >>
> >> Oops... Good catch.
> >>
> >> It's a shame we can't have unit tests whose compilation failure would
> >> indicate success, to catch issues like these.
> > 
> > Should we perhaps make use of a macro to ensure this doesn't happen?
> > something akin to the Chromium DISALLOW_COPY_AND_ASSIGN type macros?
> > 
> > (Though, deleting them, rather than making them private).
> 
> Probably also a base class can do this:
> 
> https://www.boost.org/doc/libs/1_65_0/libs/core/doc/html/core/noncopyable.html

That's an interesting idea. I gave it a try and it works fine, but it
suffers from one issue. If class B derives from class A, and class A is
made non-copyable by deriving from boost::noncopyable, then B can't also
derive from boost::noncopyable. That's fine from a behavioural point of
view, as B won't be copyable if A isn't, but it fails to record the
intent in B. If A is later refactored to become copyable while B should
stay non-copyable, there's a risk B will be overlooked.

What does everybody think, is that an acceptable risk, or would macros
be better ?

On 29/09/2020 03:30, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, Sep 25, 2020 at 05:18:36PM +0300, Tomi Valkeinen wrote:
>> On 25/09/2020 17:15, Kieran Bingham wrote:
>>> On 25/09/2020 15:07, Laurent Pinchart wrote:
>>>> On Fri, Sep 25, 2020 at 05:05:25PM +0300, Tomi Valkeinen wrote:
>>>>> FrameBufferAllocator is supposed to delete copy constructor and
>>>>> copy-assignment operator. It doesn't do that as it uses Camera as a
>>>>> parameter instead of FrameBufferAllocator.
>>>>
>>>> Oops... Good catch.
>>>>
>>>> It's a shame we can't have unit tests whose compilation failure would
>>>> indicate success, to catch issues like these.
>>>
>>> Should we perhaps make use of a macro to ensure this doesn't happen?
>>> something akin to the Chromium DISALLOW_COPY_AND_ASSIGN type macros?
>>>
>>> (Though, deleting them, rather than making them private).
>>
>> Probably also a base class can do this:
>>
>> https://www.boost.org/doc/libs/1_65_0/libs/core/doc/html/core/noncopyable.html
> 
> That's an interesting idea. I gave it a try and it works fine, but it
> suffers from one issue. If class B derives from class A, and class A is
> made non-copyable by deriving from boost::noncopyable, then B can't also
> derive from boost::noncopyable. That's fine from a behavioural point of
> view, as B won't be copyable if A isn't, but it fails to record the
> intent in B. If A is later refactored to become copyable while B should
> stay non-copyable, there's a risk B will be overlooked.
> 
> What does everybody think, is that an acceptable risk, or would macros
> be better ?

Maybe this gets a bit hacky, but it does compile:

template<class T>
class noncopyable
{
protected:
	constexpr noncopyable() = default;
private:
	noncopyable(const noncopyable&) = delete;
	noncopyable& operator=(const noncopyable&) = delete;
};

class A : noncopyable<A>
{

};

class B : public A, noncopyable<B>
{

};

Is that any better than a macro? I don't know... I'm sure it's heavier at compile time, but hopefully no overhead at runtime.

 Tomi