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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart