| Message ID | 20210211133444.764808-1-kieran.bingham@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
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. 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. - 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. 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%)