[v1,4/4] libcamera: controls: Simplify ControlValue::{ControlValue, get, set}
diff mbox series

Message ID 20250401131939.749583-5-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • libcamera: controls: Misc. changes
Related show

Commit Message

Barnabás Pőcze April 1, 2025, 1:19 p.m. UTC 
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.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/controls.h | 75 ++++++++++--------------------------
 1 file changed, 20 insertions(+), 55 deletions(-)

Comments

Laurent Pinchart April 1, 2025, 9:47 p.m. UTC | #1 
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@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,
Barnabás Pőcze April 2, 2025, 8:39 a.m. UTC | #2 
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@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,
>

Patch
diff mbox series 

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,