[libcamera-devel] [PATCH v2 03/24] libcamera: controls: Avoid exception in ControlList count() and find()

[Kieran Bingham]

Hi Laurent,

On 08/11/2019 20:53, Laurent Pinchart wrote:
> The ControlList count() and find() methods use at() to lookup the
> control numerical ID in the idmap_. This causes an exception to be
> thrown if the ID doesn't exist in the map. Fix it by using the find()
> method instead.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/controls.cpp | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 93ad2fc6a276..0c7cd449ad64 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
>   */
>  ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>  {
> -	return count(idmap_.at(id));
> +	auto iter = idmap_.find(id);
> +	if (iter == idmap_.end())
> +		return 0;
> +
> +	return count(iter->second);

Is this further count() redundant?

Has the desired item already been 'found' at that point, and thus we can
return '1'?


I fear use of std::pair once again makes this hunk fairly horrible to
read as a standalone code segment. Even more so with the user of auto
iter. (yes, ->second is my ->first pet-peeve)

Actually - Looking below, I suspect the call to count is important,
because we might be counting something else.

Can we add a brief comment here or something to make all of this clear?

Or expand the iter->second to a defined type so it's clear /what/ we're
counting.


--
Kieran

>  }
>  
>  /**
> @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>   */
>  ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
>  {
> -	return find(idmap_.at(id));
> +	auto iter = idmap_.find(id);
> +	if (iter == idmap_.end())
> +		return end();
> +
> +	return find(iter->second);
>  }
>  
>  /**
> @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
>   */
>  ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
>  {
> -	return find(idmap_.at(id));
> +	auto iter = idmap_.find(id);
> +	if (iter == idmap_.end())
> +		return end();
> +
> +	return find(iter->second);
>  }
>  
>  /**
> 

-- 
Regards
--
Kieran