[libcamera-devel] [PATCH 3/4] libcamera: device_enumerator_udev: Avoid double list lookup

[Laurent Pinchart]

Hello,

On Fri, Sep 13, 2019 at 11:19:55AM +0200, Jacopo Mondi wrote:
> On Fri, Sep 13, 2019 at 09:50:23AM +0100, Kieran Bingham wrote:
> > On 12/09/2019 21:03, Laurent Pinchart wrote:
> > > DeviceEnumeratorUdev::populateMediaDevice() searches for orphan devices
> > > in an std::list, and if found removes them using std::list::remove().
> > > This ends up looking up the entry twice. Replace the remove() call with
> > > erase() to fix it.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/device_enumerator_udev.cpp | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> > > index 210f5c1f2870..c40770911d3d 100644
> > > --- a/src/libcamera/device_enumerator_udev.cpp
> > > +++ b/src/libcamera/device_enumerator_udev.cpp
> > > @@ -182,7 +182,8 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
> > >  				       entity->deviceMinor());
> > >
> > >  		/* Take device from orphan list first, if it is in the list. */
> > > -		if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {
> > > +		auto orphan = std::find(orphans_.begin(), orphans_.end(), devnum);
> > > +		if (orphan != orphans_.end()) {
> > >  			std::string deviceNode = lookupDeviceNode(devnum);
> > >  			if (deviceNode.empty())
> > >  				return -EINVAL;
> > > @@ -191,7 +192,7 @@ int DeviceEnumeratorUdev::populateMediaDevice(const std::shared_ptr<MediaDevice>
> > >  			if (ret)
> > >  				return ret;
> > >
> > > -			orphans_.remove(devnum);
> > > +			orphans_.erase(orphan);
> >
> > Ouch, I had to look up the difference between remove and erase. That's a
> > subtle but important difference :D And I'm sure no one ever gets bitten
> > by the fact that .remove() does not 'remove' an entry from a container :-D
> 
> What difference are you referring to? My understanding is that this
> patch just saves one lookup by saving the result of std::find() in the
> orphan variable, so that it can be reused at erase time.
> 
> Am I missing something?

erase() removes the element referenced by an iterator. remove() removes
all elements whose value is identical to the passed value. As all
elements in the list should have different values the effect of both
functions should be identical, but erase() is O(1) while remove() is
O(n).

> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > >  			continue;
> > >  		}
> > >

-- 
Regards,

Laurent Pinchart