[libcamera-devel] [PATCH v2 0/6] Delete Copy-Move-Assign

[Laurent Pinchart]

Hi Kieran,

Thank you for the series.

On Thu, Feb 11, 2021 at 01:34:38PM +0000, Kieran Bingham wrote:
> Here's a series which has been niggling me on my task list since the
> topic came up which highlighted that the non-copyability of the
> FrameBufferAllocator had been incorrectly implemented.
> 
> We previously discussed providing a non-copyable class which could be inherited
> or the addition of a macro.
> 
> I've chosen to go down the macro route, because I think its clearer,
> easier to customise, and doesn't extend the inheritance (and thus
> increase the size) of classes.

Just a side note, inheriting doesn't necessarily increase the size of an
object, see https://en.cppreference.com/w/cpp/language/ebo.

> Along the way, the following tasks have occured:
> 
>  - Classes which delete their copy constructor and copy assignment
>    operator, have had those replaced by LIBCAMERA_DISABLE_COPY_AND_MOVE.
>    Where the existing copy/assignment deletion occured in public:, the
>    LIBCAMERA_DISABLE_COPY_AND_MOVE addition has been placed in private: so
>    that Doxygen does not complain. (and it should have the same effect)
> 
>  - The Buffer class deletes all of the copy, and move constructor and
>    assignment operators, so this has been kept. But as the only class
>    which goes this far, it seems to stand out on its own. I have simply
>    converted to the new usage, I didn't want to change any functionality
>    here.
> 
>  - Media objects deleted only their copy construtor. I believe this to
>    be an oversight, as if the copy constructor is deleted, then the copy
>    assignment operator should also be deleted. Therefore I see this macro
>    as a win here.

Yes, I think it was an oversight indeed. See
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-five.

>  - MappedBuffer : This did not delete it's copy constructor, and I
>    believe it should have - so I've added it. I have not done a thorough
>    search of the tree to find other instances that should also delete
>    anything yet though.

Agreed as well.

> I'm sure that more classes could be tightened up with the addition of
> these restrictions where necessary, or perhaps other variants might crop
> up. I'm not sure yet, but this can get the ball rolling in that case.
> 
> 
> Since the first v1 posting of this series, DELETE_COPY_MOVE_AND_ASSIGN
> is renamed to LIBCAMERA_DISABLE_COPY_AND_MOVE (using disable instead of
> deleted), and 'assign' has been removed from the macro names.
> 
> The extensible class was also merged during this time, so it is first
> renamed to be a generic class.h and these macros are added there.
> 
> Kieran Bingham (6):
>   libcamera: Move extensible to class
>   libcamera: class: Provide move and copy disablers
>   libcamera: buffer: Utilise DISABLE_COPY_AND_MOVE
>   libcamera: media_object: Utilise LIBCAMERA_DISABLE_COPY
>   libcamera: Utilise LIBCAMERA_DISABLE_COPY
>   libcamera: MappedBuffer: Disable copy and assignment
> 
>  include/libcamera/buffer.h                    | 10 +++----
>  include/libcamera/camera.h                    |  8 ++---
>  include/libcamera/camera_manager.h            |  6 ++--
>  include/libcamera/{extensible.h => class.h}   | 20 ++++++++++---
>  include/libcamera/controls.h                  |  7 ++---
>  include/libcamera/framebuffer_allocator.h     |  7 +++--
>  include/libcamera/internal/buffer.h           |  4 +++
>  .../libcamera/internal/byte_stream_buffer.h   |  4 +--
>  include/libcamera/internal/camera_sensor.h    |  6 ++--
>  include/libcamera/internal/file.h             |  6 ++--
>  include/libcamera/internal/log.h              |  6 +++-
>  include/libcamera/internal/media_object.h     |  9 ++++--
>  include/libcamera/internal/pipeline_handler.h |  4 +--
>  include/libcamera/internal/v4l2_subdevice.h   |  5 ++--
>  include/libcamera/internal/v4l2_videodevice.h |  6 ++--
>  include/libcamera/meson.build                 |  1 -
>  include/libcamera/request.h                   |  5 ++--
>  src/libcamera/{extensible.cpp => class.cpp}   | 29 ++++++++++++++++---
>  src/libcamera/meson.build                     |  2 +-
>  19 files changed, 94 insertions(+), 51 deletions(-)
>  rename include/libcamera/{extensible.h => class.h} (71%)
>  rename src/libcamera/{extensible.cpp => class.cpp} (85%)
> 
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,

Laurent Pinchart