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

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 02, 2020 at 11:25:25PM +0000, Kieran Bingham wrote:
> 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_; ?

It's done later in the series :-)

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

Laurent Pinchart