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

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart