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

[Barnabás Pőcze]

Hi


2025. 04. 01. 23:47 keltezéssel, Laurent Pinchart írta:
> 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 ?

Nothing depends on this change per se, but I have made use of the same uniform
handling of control values in other code. So I would like to achieve something
more uniform. Can we establish how multi-dimensional things are expected to work?


Regards,
Barnabás Pőcze

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