[libcamera-devel] [PATCH v2 6/9] libcamera: controls: List-initialize ControlList

[Laurent Pinchart]

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 at 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

-- 
Regards,

Laurent Pinchart