[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor

[Christian Rauch]

Hi Jacopo,

Thanks for reviewing this. See my responses inline below.


Am 01.04.22 um 15:28 schrieb Jacopo Mondi:
> Hi Christian
>
> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:
>> The new default constructor allows to construct a fixed-sized Span via the
>> default constructor of its stored data type.
>> This prevents the construction of empty fixed-sized Spans that cannot hold
>> any data.
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
>> ---
>>  include/libcamera/base/span.h | 5 +++++
>>  include/libcamera/controls.h  | 2 +-
>>  test/span.cpp                 | 2 +-
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h
>> index 88d2e3de..7a4806dc 100644
>> --- a/include/libcamera/base/span.h
>> +++ b/include/libcamera/base/span.h
>> @@ -112,6 +112,11 @@ public:
>>  	{
>>  	}
>>
>> +	Span()
>> +	{
>> +		Span(std::array<value_type, extent>{});
>> +	}
>> +
>
> This constructor creates a span of 'extent' elements all of them
> initialized to 0 then ?

All elements will be default constructed. Integers and floats will be
initialised to 0, "std::strings" will be empty, and 'Size' and
'Rectangle' will use their specified default constructors.
The default initialisation is required because the fixed-sized array
cannot be empty.

>
> If I remove it I get
>
> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’
>   380 |                         return T{};
>
> Caused by
> 	template<typename T>
> 	T get(const Control<T> &ctrl) const
> 	{
> 		const ControlValue *val = find(ctrl.id());
> 		if (!val)
> 			return T{}; <------
>
> 		return val->get<T>();
> 	}
>
> as now that extent is != 0 the expression doesn't bind anymore to the
> existing constructor:
>
> 	template<bool Dependent = false,
> 		 typename = std::enable_if_t<Dependent || Extent == 0>>
> 	constexpr Span() noexcept
> 		: data_(nullptr)
> 	{
> 	}
>
> (I don't fully get what is the use of Dependent in the above definition :)

I also do not understand the use of this constructor, since a
0-fixed-size Span cannot store anything.
>
> However I feel like creating a Span of Extent elements all zeroed is a
> bit hill-defined, considering it is only used to return an error
> condition. In example, if I'm not mistaken a Span constructed with
> your proposed constructor will
>
> 	constexpr size_type size() const noexcept { return Extent; }
> 	constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }
> 	constexpr bool empty() const noexcept { return size() == 0; }
>
> be !empty and of size == 2, which is misleading considering we really
> want to return an empty Span in case of error and the caller should
> not be mis-leaded thinking it is valid.
>
> I don't have any super smart proposal to offer, if not recording the
> 'valid' or 'empty' state in a class member and use it in size() and
> empty() instead of relying on Extent which is defined at overload
> resolution time and cannot be assigned at run time.

I think this is a general issue with return default constructed objects
to indicate an error. The error is only defined implicitly. If
ControlList::get returns a 'Size' with width=0 and height=0, then it is
not clear if this indicates an error or if this value is really stored.

You could add special class members to indicate that an object is
"valid", but this is not a nice solution and only works for custom types
in libcamera. The "C" way would probably to indicate an error by a NULL
pointer and a returned error number.

I think the proper way would be to throw an exception. This could also
be a custom libcamera exception.

>
>>  	explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
>>  		: data_(ptr)
>>  	{
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 665bcac1..de8a7770 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -167,7 +167,7 @@ public:
>>
>>  		using V = typename T::value_type;
>>  		const V *value = reinterpret_cast<const V *>(data().data());
>> -		return { value, numElements_ };
>> +		return T{ value, numElements_ };
>
> this applies to overload
>
> 	template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> 						       std::is_same<std::string, std::remove_cv_t<T>>::value,
> 	T get() const
> 	{
> 		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> 		assert(isArray_);
>
> 		using V = typename T::value_type;
> 		const V *value = reinterpret_cast<const V *>(data().data());
> 		return { value, numElements_ };
> 	}
>
> Isn't the returned { value, numElements_ } already used to construct an
> instance of T if assigned to an lvalue expression ?
>
> IOW why do you need to contruct an instance of T and return it
> explicitely ? Thanks to return value optimization this shouldn't have
> any performance impact but I have missed why it is needed.
>
> <2 minutes later>
>
> And now that I wrote this, if I remove it I get
>
> include/libcamera/controls.h:170:46: error: converting to
> ‘libcamera::Span<const float, 2>’ from initializer list would use
> explicit constructor ‘constexpr libcamera::Span<T,
> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T,
> Extent>::size_type) [with T = const float; long unsigned int Extent =
> 2; libcamera::Span<T, Extent>::pointer = const float*;
> libcamera::Span<T, Extent>::size_type = long unsigned int]’
>
> Because indeed
>       explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
>
> is, well, explicit (which I think it's ok and should not be removed)
>
> I'll leave here the above text anyway for others that might have the
> same question :)
>
>>  	}
>>
>>  #ifndef __DOXYGEN__
>> diff --git a/test/span.cpp b/test/span.cpp
>> index abf3a5d6..c37e2a66 100644
>> --- a/test/span.cpp
>> +++ b/test/span.cpp
>> @@ -37,7 +37,7 @@ protected:
>>  		 * to generate undefined behaviour.
>>  		 */
>>
>> -		Span<int, 0>{};
>> +		/* Span<int, 0>{}; */
>
> You should remove it, or if the above issue with constructing an
> 'empty' Span of size > 0 is resolved, prove that it works as intended.

I commented it out instead of removing this, because the comment above
says "[...] Commented-out tests are expected not to compile [...]" and
there was already a commented case for an 4-sized Span. Anyway, I don't
see a real application of a 0-fixed-sized Span, as it will never be able
to store anything.

Best,
Christian


>
>                 Span<int, 4> s{}
>                 if (!s.empty())
>                         error!
>
> Thanks
>    j
>
>>  		/* Span<int, 4>{}; */
>>
>>  		Span<int, 4>{ &i[0], 4 };
>> --
>> 2.25.1
>>