| Message ID | 20200229164254.23604-13-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> I'll put this here, but that 'bool_' reference is annoying below: Reviewed-by: Kieran Bingham <kieran.bingham@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 >
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@ideasonboard.com> > > I'll put this here, but that 'bool_' reference is annoying below: > > Reviewed-by: Kieran Bingham <kieran.bingham@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
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_); + } + template<typename T> - void set(const T &value); + void set(const T &value) + { + type_ = details::control_type<std::remove_cv_t<T>>::value; + *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
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@ideasonboard.com> --- include/libcamera/controls.h | 15 ++++++++++-- src/libcamera/controls.cpp | 47 ------------------------------------ 2 files changed, 13 insertions(+), 49 deletions(-)