Message ID | 20201218164754.81422-7-jacopo@jmondi.org |
---|---|
State | Superseded |
Delegated to: | Jacopo Mondi |
Headers | show |
Series |
|
Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Dec 18, 2020 at 05:47:51PM +0100, Jacopo Mondi wrote: > Provide an additional constructor to the ControlList class to > allow the construction of instances of the class with the usage > of an std::intializer_list<> of ControlId and an associated > ControlValue. > > The new constructor allows to intialize a ControlList instance s/intialize/initialize/ > at construction time, allowing the declaration of populated > ControlList instances. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 2 ++ > src/libcamera/controls.cpp | 11 +++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 3634dc431618..4df6615a3c7b 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -348,9 +348,11 @@ class ControlList > { > private: > using ControlListMap = std::unordered_map<unsigned int, ControlValue>; > + using ControlsMap = std::unordered_map<const ControlId *, ControlValue>; As the ControlId should never be null, should this be a reference ? > > public: > ControlList(); > + ControlList(std::initializer_list<ControlsMap::value_type> controls); > ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr); > ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr); > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index c58ed3946f3b..7650057dde7d 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -797,6 +797,17 @@ ControlList::ControlList() > { > } > > +/** > + * \brief Construct a ControlList with a list of values > + * \param[in] controls The controls and associated values to initialize the list with > + */ > +ControlList::ControlList(std::initializer_list<ControlsMap::value_type> controls) > + : ControlList() > +{ > + for (const auto &ctrl : controls) > + set(ctrl.first->id(), ctrl.second); The annoying part here is to we lose type safety. The ControlValue in the controls variable is constructor without any type inference based on a Control<T>. This leads to the explicit construction of, for instance, Span<const float>, in patch 7/9. Is there a way we could do better ? > +} > + > /** > * \brief Construct a ControlList with an optional control validator > * \param[in] idmap The ControlId map for the control list target object
Hi Laurent, On Wed, Dec 23, 2020 at 07:56:50AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Dec 18, 2020 at 05:47:51PM +0100, Jacopo Mondi wrote: > > Provide an additional constructor to the ControlList class to > > allow the construction of instances of the class with the usage > > of an std::intializer_list<> of ControlId and an associated > > ControlValue. > > > > The new constructor allows to intialize a ControlList instance > > s/intialize/initialize/ > > > at construction time, allowing the declaration of populated > > ControlList instances. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/controls.h | 2 ++ > > src/libcamera/controls.cpp | 11 +++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 3634dc431618..4df6615a3c7b 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -348,9 +348,11 @@ class ControlList > > { > > private: > > using ControlListMap = std::unordered_map<unsigned int, ControlValue>; > > + using ControlsMap = std::unordered_map<const ControlId *, ControlValue>; > > As the ControlId should never be null, should this be a reference ? > > > > > public: > > ControlList(); > > + ControlList(std::initializer_list<ControlsMap::value_type> controls); > > ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr); > > ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr); > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index c58ed3946f3b..7650057dde7d 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -797,6 +797,17 @@ ControlList::ControlList() > > { > > } > > > > +/** > > + * \brief Construct a ControlList with a list of values > > + * \param[in] controls The controls and associated values to initialize the list with > > + */ > > +ControlList::ControlList(std::initializer_list<ControlsMap::value_type> controls) > > + : ControlList() > > +{ > > + for (const auto &ctrl : controls) > > + set(ctrl.first->id(), ctrl.second); > > The annoying part here is to we lose type safety. The ControlValue in > the controls variable is constructor without any type inference based on > a Control<T>. This leads to the explicit construction of, for instance, > Span<const float>, in patch 7/9. Is there a way we could do better ? > I'll keep the patch like it is right now in next version as I've got so many piled up patches this seems like a minor issue, as the only use case for statically initialized ControlList is the sensor database current implementation. It can be changed on top if desired. > > +} > > + > > /** > > * \brief Construct a ControlList with an optional control validator > > * \param[in] idmap The ControlId map for the control list target object > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 3634dc431618..4df6615a3c7b 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -348,9 +348,11 @@ class ControlList { private: using ControlListMap = std::unordered_map<unsigned int, ControlValue>; + using ControlsMap = std::unordered_map<const ControlId *, ControlValue>; public: ControlList(); + ControlList(std::initializer_list<ControlsMap::value_type> controls); ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr); ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr); diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index c58ed3946f3b..7650057dde7d 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -797,6 +797,17 @@ ControlList::ControlList() { } +/** + * \brief Construct a ControlList with a list of values + * \param[in] controls The controls and associated values to initialize the list with + */ +ControlList::ControlList(std::initializer_list<ControlsMap::value_type> controls) + : ControlList() +{ + for (const auto &ctrl : controls) + set(ctrl.first->id(), ctrl.second); +} + /** * \brief Construct a ControlList with an optional control validator * \param[in] idmap The ControlId map for the control list target object
Provide an additional constructor to the ControlList class to allow the construction of instances of the class with the usage of an std::intializer_list<> of ControlId and an associated ControlValue. The new constructor allows to intialize a ControlList instance at construction time, allowing the declaration of populated ControlList instances. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/controls.h | 2 ++ src/libcamera/controls.cpp | 11 +++++++++++ 2 files changed, 13 insertions(+)