[PATCH v7 06/18] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class

[Milan Zamazal]

Hi Laurent,

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan and Andrei,
>
> Thank you for the patch.
>
> On Thu, Apr 04, 2024 at 10:46:43AM +0200, Milan Zamazal wrote:
>> From: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> 
>> The SharedMemObject class template contains a fair amount of inline code that
>
> The standard line length for commit messages is 72. Out of curiosity,
> are you using an editor that has a different setting by default ?

Yes.  Magit used to set it to 72 but due to complaints from users who have
different project policies, it stopped doing so and leaves it at whatever value
is set.  I'll change it in my environment to 72 for libcamera commit messages.

>> does not depend on the template types T. To avoid duplicating it in every
>> template specialization, split that code to a separate base SharedMem class.
>> 
>> We don't define copy semantics for the classes (we don't need one at the moment)
>> and we make them non-copyable since the default copy constructor would lead to
>> use-after-unmap.
>> 
>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>> 
>> Reviewed-by: Pavel Machek <pavel at ucw.cz>
>> Co-developed-by: Dennis Bonke <admin at dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk at gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  .../libcamera/internal/shared_mem_object.h    | 108 ++++----
>>  src/libcamera/meson.build                     |   1 +
>>  src/libcamera/shared_mem_object.cpp           | 244 ++++++++++++++++++
>>  3 files changed, 303 insertions(+), 50 deletions(-)
>>  create mode 100644 src/libcamera/shared_mem_object.cpp
>> 
>> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
>> index 8f2e7120..b2ab9ad1 100644
>> --- a/include/libcamera/internal/shared_mem_object.h
>> +++ b/include/libcamera/internal/shared_mem_object.h
>> @@ -1,85 +1,103 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>> - * Copyright (C) 2023, Raspberry Pi Ltd
>> + * Copyright (C) 2023 Raspberry Pi Ltd
>> + * Copyright (C) 2024 Andrei Konovalov
>> + * Copyright (C) 2024 Dennis Bonke
>>   *
>> - * shared_mem_object.h - Helper class for shared memory allocations
>> + * shared_mem_object.h - Helpers for shared memory allocations
>>   */
>>  #pragma once
>>  
>> -#include <fcntl.h>
>>  #include <stddef.h>
>> +#include <stdint.h>
>>  #include <string>
>>  #include <sys/mman.h>
>> -#include <sys/stat.h>
>> -#include <unistd.h>
>> +#include <type_traits>
>>  #include <utility>
>>  
>>  #include <libcamera/base/class.h>
>>  #include <libcamera/base/shared_fd.h>
>> +#include <libcamera/base/span.h>
>>  
>>  namespace libcamera {
>>  
>> -template<class T>
>> -class SharedMemObject
>> +class SharedMem
>>  {
>>  public:
>> -	static constexpr std::size_t size = sizeof(T);
>> +	using span = Span<uint8_t>;
>
> The type would need to be named Span, but I think it should be dropped,
> it doesn't seem to increase readability to me.

I haven't done this for readability but to have the type specification in a
single place.  But yes, in this case the type is given and if we decide to
change it to something else (e.g. C++20 std::span) then it'll be handled by a
mass update.  So I can drop the definition.