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

Message ID 20191108205409.18845-4-laurent.pinchart@ideasonboard.com
State Accepted
Delegated to: Laurent Pinchart
Headers show
Series
  • Control serialization and IPA C API
Related show

Commit Message

Laurent Pinchart Nov. 8, 2019, 8:53 p.m. UTC
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@ideasonboard.com>
---
 src/libcamera/controls.cpp | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Niklas Söderlund Nov. 14, 2019, 8:13 a.m. UTC | #1
Hi Laurent,

Thanks for your work.

On 2019-11-08 22:53:48 +0200, 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@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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);
>  }
>  
>  /**
> @@ -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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Nov. 15, 2019, 4 p.m. UTC | #2
Hi Laurent,

On Fri, Nov 08, 2019 at 10:53:48PM +0200, 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.

Be aware that ControlList::at() (const) still uses at() and could
potentially raise an exception as well, while I think we clearly
prescribes that libcamera does not uses them.

If that's fine please have mine
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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);
>  }
>
>  /**
> @@ -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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Nov. 18, 2019, 12:44 a.m. UTC | #3
Hi Jacopo,

On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote:
> On Fri, Nov 08, 2019 at 10:53:48PM +0200, 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.
> 
> Be aware that ControlList::at() (const) still uses at() and could
> potentially raise an exception as well, while I think we clearly
> prescribes that libcamera does not uses them.

There's not much we can do about that, as at() returns a reference, so
there's no way to return an error. It's an error in the caller to use
at() without checking if the item exists first. We could specify this in
the documentation, or maybe remove the at() method completely, forcing
the caller to use find(). What do you think is best ?

> If that's fine please have mine
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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);
> >  }
> >
> >  /**
> > @@ -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);
> >  }
> >
> >  /**
Jacopo Mondi Nov. 18, 2019, 10:51 p.m. UTC | #4
Hi Laurent,

On Mon, Nov 18, 2019 at 02:44:54AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote:
> > On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote:
> > > The ControlList count() and find() methods use at() to lookup the

s/ControlList/ControlInfoMap/

> > > 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.
> >
> > Be aware that ControlList::at() (const) still uses at() and could
> > potentially raise an exception as well, while I think we clearly
> > prescribes that libcamera does not uses them.
>
> There's not much we can do about that, as at() returns a reference, so
> there's no way to return an error. It's an error in the caller to use
> at() without checking if the item exists first. We could specify this in
> the documentation, or maybe remove the at() method completely, forcing
> the caller to use find(). What do you think is best ?
>

I don't see users of ControlInfoMap::at() if not the the
ControlInfoMap class itself. I would be fine in dropping it. There are
usaged of at() on maps in many places on std::map instances though, so
you don't want to remove it for consitency, that's fine as well.

> > If that's fine please have mine
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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);
> > >  }
> > >
> > >  /**
> > > @@ -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
Laurent Pinchart Nov. 19, 2019, 12:35 a.m. UTC | #5
Hi Jacopo,

On Mon, Nov 18, 2019 at 11:51:21PM +0100, Jacopo Mondi wrote:
> On Mon, Nov 18, 2019 at 02:44:54AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote:
> > > On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote:
> > > > The ControlList count() and find() methods use at() to lookup the
> 
> s/ControlList/ControlInfoMap/
> 
> > > > 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.
> > >
> > > Be aware that ControlList::at() (const) still uses at() and could
> > > potentially raise an exception as well, while I think we clearly
> > > prescribes that libcamera does not uses them.
> >
> > There's not much we can do about that, as at() returns a reference, so
> > there's no way to return an error. It's an error in the caller to use
> > at() without checking if the item exists first. We could specify this in
> > the documentation, or maybe remove the at() method completely, forcing
> > the caller to use find(). What do you think is best ?
> 
> I don't see users of ControlInfoMap::at() if not the the
> ControlInfoMap class itself. I would be fine in dropping it. There are
> usaged of at() on maps in many places on std::map instances though, so
> you don't want to remove it for consitency, that's fine as well.

I think there's at least one user in the tests, as compilation failed
when I dropped the method.

Would you like to submit a patch, or do you think this is overkill ?

> > > If that's fine please have mine
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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);
> > > >  }
> > > >
> > > >  /**
> > > > @@ -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);
> > > >  }
> > > >
> > > >  /**
Kieran Bingham Nov. 19, 2019, 3:46 p.m. UTC | #6
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@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);
>  }
>  
>  /**
>
Laurent Pinchart Nov. 20, 2019, 3:48 a.m. UTC | #7
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@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);
> >  }
> >  
> >  /**
Kieran Bingham Nov. 20, 2019, 9:17 a.m. UTC | #8
Hi Laurent,

On 20/11/2019 03:48, Laurent Pinchart wrote:
> 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@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.

Yes, the use of local variables to make the naming explicit is what I
meant here ...

I feel like any use of ->first and ->second is the equivalent of naming
variables
  int integer = 0;

Just like the std::pair, here there is an integer, but it doesn't tell
me what it stores it for, The ->first ->second /are/ the first and
second parameter of the pair - but that doesn't tell me what they are
used for. And it leaves the reader blind (or worse, confused).

I feel strongly enough about this to say we should propose a coding
standard addition to say all use of std::pair should be named explicitly
by mapping to typed (and well named) local variables (which I expect the
compiler to optimise out), with perhaps an exception if the std::pair
return types are already exposed within the local function and "might"
be redundant.

What do you think ?

>> 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);


That sounds even better.
Perhaps something like this then (Adjust as appropriate of course):

ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
{
 /*
  * The idmap is generated with one entry per entry in ControlInfoMap
  * with a 1:1 mapping between the two, therefore we count the relevant
  * idmap entries to establish the ControlInfoMap count without
  * performing an additional lookup.
  */

  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);
>>>  }
>>>  
>>>  /**
>
Laurent Pinchart Nov. 21, 2019, 7:46 a.m. UTC | #9
Hi Kieran,

On Wed, Nov 20, 2019 at 09:17:05AM +0000, Kieran Bingham wrote:
> On 20/11/2019 03:48, Laurent Pinchart wrote:
> > 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@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.
> 
> Yes, the use of local variables to make the naming explicit is what I
> meant here ...
> 
> I feel like any use of ->first and ->second is the equivalent of naming
> variables
>   int integer = 0;
> 
> Just like the std::pair, here there is an integer, but it doesn't tell
> me what it stores it for, The ->first ->second /are/ the first and
> second parameter of the pair - but that doesn't tell me what they are
> used for. And it leaves the reader blind (or worse, confused).
> 
> I feel strongly enough about this to say we should propose a coding
> standard addition to say all use of std::pair should be named explicitly
> by mapping to typed (and well named) local variables (which I expect the
> compiler to optimise out), with perhaps an exception if the std::pair
> return types are already exposed within the local function and "might"
> be redundant.
> 
> What do you think ?

I agree with you, and we're already doing this in most cases. I would
however relax the rule a little bit, in some cases using .first and
.second is explicit enough, but that's far from being the general rule.

One case where .first and .second is fine in my opinion is when they
really denote the first and second instances of something, with an
explicit std::pair<Type, Type>.

	/* French fries have to be fried twice. */
	std::pair<std::chrono::duration, std::chrono::duration> cookingDuration;
	fry(fries, cookingTime.first);
	std::thread::wait_for(restDuration);
	fry(fries, cookingTime.second);

Another case could be when the map type is explicitly shown in the code
iterating on the map. I find the following explicit enough:

	std::map<int, std::string> theMap = generateTheMap();

	for (const auto &pair : theMap)
		std::cout << "mapping from " << pair.first
			  << " to " << pair.second << std::endl;

But this wouldn't be explicit enough:

	for (const auto &pair : theMap_)
		std::cout << "mapping from " << pair.first
			  << " to " << pair.second << std::endl;

as the definition of theMap_ is located in a different place.

> >> 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);
> 
> That sounds even better.
> Perhaps something like this then (Adjust as appropriate of course):
> 
> ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> {
>  /*
>   * The idmap is generated with one entry per entry in ControlInfoMap
>   * with a 1:1 mapping between the two, therefore we count the relevant
>   * idmap entries to establish the ControlInfoMap count without
>   * performing an additional lookup.
>   */
> 
>   return idmap_.count(id);
> }

I've applied a slightly modified version. Thank you for the proposal.

> >> 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);
> >>>  }
> >>>  
> >>>  /**

Patch

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);
 }
 
 /**
@@ -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);
 }
 
 /**