| Message ID | 20190912200330.19004-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, 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@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 Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > continue; > } > >
Hi Kieran, On Fri, Sep 13, 2019 at 09:50:23AM +0100, Kieran Bingham wrote: > Hi Laurent, > > 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@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? > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > continue; > > } > > > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@ideasonboard.com> > > > > > continue; > > > } > > >
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); continue; }
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@ideasonboard.com> --- src/libcamera/device_enumerator_udev.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)