[libcamera-devel] [PATCH 12/31] libcamera: controls: Move ControlValue get() and set() to controls.h

[Kieran Bingham]

Hi Laurent,

On 29/02/2020 16:42, Laurent Pinchart wrote:
> To avoid defining all specializations of ControlValue::get() and
> ControlValue::set() manually, move the definition of those functions to
> controls.h.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


I'll put this here, but that 'bool_' reference is annoying below:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  include/libcamera/controls.h | 15 ++++++++++--
>  src/libcamera/controls.cpp   | 47 ------------------------------------
>  2 files changed, 13 insertions(+), 49 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 429f01b0fd24..39e240438861 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -8,6 +8,7 @@
>  #ifndef __LIBCAMERA_CONTROLS_H__
>  #define __LIBCAMERA_CONTROLS_H__
>  
> +#include <assert.h>
>  #include <string>
>  #include <unordered_map>
>  
> @@ -70,9 +71,19 @@ public:
>  	}
>  
>  	template<typename T>
> -	T get() const;
> +	T get() const
> +	{
> +		assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> +
> +		return *reinterpret_cast<const T *>(&bool_);

I don't like how all cases now look like they use a bool.

The union is feeling quite pointless here...
Perhaps remove the union and just store int64_t value_; ?


> +	}
> +
>  	template<typename T>
> -	void set(const T &value);
> +	void set(const T &value)
> +	{
> +		type_ = details::control_type<std::remove_cv_t<T>>::value;

Aha nice :-)

> +		*reinterpret_cast<T *>(&bool_) = value;
> +	}
>  
>  private:
>  	ControlType type_;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 6a0d66fbea8d..f3d79785e6a8 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -175,53 +175,6 @@ bool ControlValue::operator==(const ControlValue &other) const
>   * \param[in] value The control value
>   */
>  
> -#ifndef __DOXYGEN__
> -template<>
> -bool ControlValue::get<bool>() const
> -{
> -	ASSERT(type_ == ControlTypeBool);
> -
> -	return bool_;
> -}
> -
> -template<>
> -int32_t ControlValue::get<int32_t>() const
> -{
> -	ASSERT(type_ == ControlTypeInteger32);
> -
> -	return integer32_;
> -}
> -
> -template<>
> -int64_t ControlValue::get<int64_t>() const
> -{
> -	ASSERT(type_ == ControlTypeInteger64);
> -
> -	return integer64_;
> -}
> -
> -template<>
> -void ControlValue::set<bool>(const bool &value)
> -{
> -	type_ = ControlTypeBool;
> -	bool_ = value;
> -}
> -
> -template<>
> -void ControlValue::set<int32_t>(const int32_t &value)
> -{
> -	type_ = ControlTypeInteger32;
> -	integer32_ = value;
> -}
> -
> -template<>
> -void ControlValue::set<int64_t>(const int64_t &value)
> -{
> -	type_ = ControlTypeInteger64;
> -	integer64_ = value;
> -}
> -#endif /* __DOXYGEN__ */
> -
>  /**
>   * \class ControlId
>   * \brief Control static metadata
> 

-- 
Regards
--
Kieran