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

[Kieran Bingham]

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

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

-- 
Regards
--
Kieran