[libcamera-devel] [PATCH] FrameBufferAllocator: fix non-copyability

[Tomi Valkeinen]

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