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

[Laurent Pinchart]

Hi Kieran,

On Tue, Nov 19, 2019 at 03:46:12PM +0000, Kieran Bingham wrote:
> 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)

:-) I dislike it as well, but in this case it's nothing we can control.
idmap_ is an std::unordered_map, and std::unordered_map::value_type is a
std::pair. That's the C++ STL API. What we can do is use local variables
to make this more explicit.

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

Actually I don't think so. The idmap is generated with one entry per
entry in ControlInfoMap, with a 1:1 mapping between the two, so I think
we can just return 1 here. Or even better,

	return idmap_.count(id);

> 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.
> 
> >  }
> >  
> >  /**
> > @@ -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,

Laurent Pinchart