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

[Kieran Bingham]

On 11/02/2021 20:55, Laurent Pinchart wrote:
> 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.

Good point, somehow I had it in my head that the alternative approach
extended the size of the class somehow, but if the objects are the same
size, it can just be a 'cast'.



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


Interesting seeing that it didn't actually 'need' it, but I think I
agree, making it explicit helps readability.

--
Kieran


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