[PATCH v1 4/4] libcamera: controls: Simplify ControlValue::{ControlValue, get, set}

[Laurent Pinchart]

Hi Barnabás,

Thank you for the patch.

On Tue, Apr 01, 2025 at 03:19:39PM +0200, Barnabás Pőcze wrote:
> Whether or not the control has an array type can be determined from the
> static information in `detail::control_type<T>`, so use that and
> `if constexpr` to deduplicate the constructor, get, and set functions.

I'd rather not do this, as it would prevent us from supporting arrays of
strings. Is there anything you're working on that depends on this
series, or is it standalone cleanups ?

> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  include/libcamera/controls.h | 75 ++++++++++--------------------------
>  1 file changed, 20 insertions(+), 55 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 85c724ec1..22b5e3d96 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -136,28 +136,14 @@ public:
>  	ControlValue();
>  
>  #ifndef __DOXYGEN__
> -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> -					      details::control_type<T>::value &&
> -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -	ControlValue(const T &value)
> -		: type_(ControlTypeNone), numElements_(0)
> -	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> -		    &value, 1, sizeof(T));
> -	}
> -
> -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> +	template<typename T, typename = std::void_t<decltype(details::control_type<T>::value)>>
>  #else
>  	template<typename T>
>  #endif
>  	ControlValue(const T &value)
> -		: type_(ControlTypeNone), numElements_(0)
> +		: type_(ControlTypeNone), isArray_(false), numElements_(0)
>  	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, true,
> -		    value.data(), value.size(), sizeof(typename T::value_type));
> +		set(value);
>  	}
>  
>  	~ControlValue();
> @@ -180,54 +166,33 @@ public:
>  		return !(*this == other);
>  	}
>  
> -#ifndef __DOXYGEN__
> -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -	T get() const
> -	{
> -		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> -		assert(!isArray_);
> -
> -		return *reinterpret_cast<const T *>(data().data());
> -	}
> -
> -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -#else
>  	template<typename T>
> -#endif
>  	T get() const
>  	{
> -		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> -		assert(isArray_);
> +		using TypeInfo = details::control_type<std::remove_cv_t<T>>;
>  
> -		using V = typename T::value_type;
> -		const V *value = reinterpret_cast<const V *>(data().data());
> -		return T{ value, numElements_ };
> -	}
> +		assert(type_ == TypeInfo::value);
> +		assert(isArray_ == (TypeInfo::size > 0));
>  
> -#ifndef __DOXYGEN__
> -	template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> -					      !std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -	void set(const T &value)
> -	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, false,
> -		    reinterpret_cast<const void *>(&value), 1, sizeof(T));
> +		if constexpr (TypeInfo::size > 0)
> +			return T(reinterpret_cast<const typename T::value_type *>(data().data()), numElements_);
> +		else
> +			return *reinterpret_cast<const T *>(data().data());
>  	}
>  
> -	template<typename T, std::enable_if_t<details::is_span<T>::value ||
> -					      std::is_same<std::string, std::remove_cv_t<T>>::value,
> -					      std::nullptr_t> = nullptr>
> -#else
>  	template<typename T>
> -#endif
>  	void set(const T &value)
>  	{
> -		set(details::control_type<std::remove_cv_t<T>>::value, true,
> -		    value.data(), value.size(), sizeof(typename T::value_type));
> +		using TypeInfo = details::control_type<std::remove_cv_t<T>>;
> +		constexpr auto type = TypeInfo::value;
> +
> +		if constexpr (TypeInfo::size > 0) {
> +			static_assert(std::is_trivially_copyable_v<typename T::value_type>);
> +			set(type, true, value.data(), value.size(), sizeof(typename T::value_type));
> +		} else {
> +			static_assert(std::is_trivially_copyable_v<T>);
> +			set(type, false, reinterpret_cast<const void *>(&value), 1, sizeof(T));
> +		}
>  	}
>  
>  	void reserve(ControlType type, bool isArray = false,

-- 
Regards,

Laurent Pinchart