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

[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 ?

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 :)

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.

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

                Span<int, 4> s{}
                if (!s.empty())
                        error!

Thanks
   j

>  		/* Span<int, 4>{}; */
>
>  		Span<int, 4>{ &i[0], 4 };
> --
> 2.25.1
>