| Message ID | 20210515040511.23294-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2021-05-15 07:05:09 +0300, Laurent Pinchart wrote: > Use the newly introduced utils::enumerate() to replace manual loop > counters. A local variable is needed, as utils::enumerate() requires an > lvalue reference. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > Changes since v1: > > - Use structured bindings > --- > src/v4l2/v4l2_compat_manager.cpp | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index 90c0f0121a32..96dbcdf28f04 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -23,6 +23,7 @@ > #include <libcamera/camera_manager.h> > > #include "libcamera/internal/log.h" > +#include "libcamera/internal/utils.h" > > #include "v4l2_camera_file.h" > > @@ -81,11 +82,10 @@ int V4L2CompatManager::start() > * For each Camera registered in the system, a V4L2CameraProxy gets > * created here to wrap a camera device. > */ > - unsigned int index = 0; > - for (auto &camera : cm_->cameras()) { > + auto cameras = cm_->cameras(); > + for (auto [index, camera] : utils::enumerate(cameras)) { > V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); > proxies_.emplace_back(proxy); > - ++index; > } > > return 0; > @@ -117,11 +117,10 @@ int V4L2CompatManager::getCameraIndex(int fd) > if (!target) > return -1; > > - unsigned int index = 0; > - for (auto &camera : cm_->cameras()) { > + auto cameras = cm_->cameras(); > + for (auto [index, camera] : utils::enumerate(cameras)) { > if (camera == target) > return index; > - ++index; > } > > return -1; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 15/05/2021 05:05, Laurent Pinchart wrote: > Use the newly introduced utils::enumerate() to replace manual loop > counters. A local variable is needed, as utils::enumerate() requires an > lvalue reference. So this is not possible? for (auto [index, camera] : utils::enumerate(cm_->cameras()) { I guess that's fine, but will it be obvious to someone trying to use it that they need to move the thing to enumerate to a local variable if it's not there? I probably really don't want to know - but why does it need this? It seems like an odd limitation, but if it's just a small corner case, then I don't mind having it split out. It helps keep line lengths shorter anyway...? Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Use structured bindings > --- > src/v4l2/v4l2_compat_manager.cpp | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > index 90c0f0121a32..96dbcdf28f04 100644 > --- a/src/v4l2/v4l2_compat_manager.cpp > +++ b/src/v4l2/v4l2_compat_manager.cpp > @@ -23,6 +23,7 @@ > #include <libcamera/camera_manager.h> > > #include "libcamera/internal/log.h" > +#include "libcamera/internal/utils.h" > > #include "v4l2_camera_file.h" > > @@ -81,11 +82,10 @@ int V4L2CompatManager::start() > * For each Camera registered in the system, a V4L2CameraProxy gets > * created here to wrap a camera device. > */ > - unsigned int index = 0; > - for (auto &camera : cm_->cameras()) { > + auto cameras = cm_->cameras(); > + for (auto [index, camera] : utils::enumerate(cameras)) { > V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); > proxies_.emplace_back(proxy); > - ++index; > } > > return 0; > @@ -117,11 +117,10 @@ int V4L2CompatManager::getCameraIndex(int fd) > if (!target) > return -1; > > - unsigned int index = 0; > - for (auto &camera : cm_->cameras()) { > + auto cameras = cm_->cameras(); > + for (auto [index, camera] : utils::enumerate(cameras)) { > if (camera == target) > return index; > - ++index; > } > > return -1; >
Hi Kieran, On Mon, May 17, 2021 at 01:54:08PM +0100, Kieran Bingham wrote: > On 15/05/2021 05:05, Laurent Pinchart wrote: > > Use the newly introduced utils::enumerate() to replace manual loop > > counters. A local variable is needed, as utils::enumerate() requires an > > lvalue reference. > > So this is not possible? > > for (auto [index, camera] : utils::enumerate(cm_->cameras()) { Unfortunately not :-( > I guess that's fine, but will it be obvious to someone trying to use it > that they need to move the thing to enumerate to a local variable if > it's not there? The compiler should make it obvious: ../../src/v4l2/v4l2_compat_manager.cpp: In member function ‘int V4L2CompatManager::start()’: ../../src/v4l2/v4l2_compat_manager.cpp:85:53: error: no matching function for call to ‘enumerate(std::vector<std::shared_ptr<libcamera::Camera> >)’ 85 | for (auto [index, camera] : utils::enumerate(cm_->cameras())) { utils::enumerate() takes an lvalue reference, and cm_->cameras() is an rvalue. > I probably really don't want to know - but why does it need this? https://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression cm_->cameras() returns a temporay value, which is passed to utils::enumerate(). utils::enumerate() returns another temporary that stores a reference to cm_->cameras(). The lifetime of the second temporary is extended to the whole loop, but the lifetime of the first temporary isn't. > It seems like an odd limitation, but if it's just a small corner case, > then I don't mind having it split out. It helps keep line lengths > shorter anyway...? > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Use structured bindings > > --- > > src/v4l2/v4l2_compat_manager.cpp | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > index 90c0f0121a32..96dbcdf28f04 100644 > > --- a/src/v4l2/v4l2_compat_manager.cpp > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > @@ -23,6 +23,7 @@ > > #include <libcamera/camera_manager.h> > > > > #include "libcamera/internal/log.h" > > +#include "libcamera/internal/utils.h" > > > > #include "v4l2_camera_file.h" > > > > @@ -81,11 +82,10 @@ int V4L2CompatManager::start() > > * For each Camera registered in the system, a V4L2CameraProxy gets > > * created here to wrap a camera device. > > */ > > - unsigned int index = 0; > > - for (auto &camera : cm_->cameras()) { > > + auto cameras = cm_->cameras(); > > + for (auto [index, camera] : utils::enumerate(cameras)) { > > V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); > > proxies_.emplace_back(proxy); > > - ++index; > > } > > > > return 0; > > @@ -117,11 +117,10 @@ int V4L2CompatManager::getCameraIndex(int fd) > > if (!target) > > return -1; > > > > - unsigned int index = 0; > > - for (auto &camera : cm_->cameras()) { > > + auto cameras = cm_->cameras(); > > + for (auto [index, camera] : utils::enumerate(cameras)) { > > if (camera == target) > > return index; > > - ++index; > > } > > > > return -1;
Hi Kieran, On Mon, May 17, 2021 at 07:36:43PM +0300, Laurent Pinchart wrote: > On Mon, May 17, 2021 at 01:54:08PM +0100, Kieran Bingham wrote: > > On 15/05/2021 05:05, Laurent Pinchart wrote: > > > Use the newly introduced utils::enumerate() to replace manual loop > > > counters. A local variable is needed, as utils::enumerate() requires an > > > lvalue reference. > > > > So this is not possible? > > > > for (auto [index, camera] : utils::enumerate(cm_->cameras()) { > > Unfortunately not :-( > > > I guess that's fine, but will it be obvious to someone trying to use it > > that they need to move the thing to enumerate to a local variable if > > it's not there? > > The compiler should make it obvious: > > ../../src/v4l2/v4l2_compat_manager.cpp: In member function ‘int V4L2CompatManager::start()’: > ../../src/v4l2/v4l2_compat_manager.cpp:85:53: error: no matching function for call to ‘enumerate(std::vector<std::shared_ptr<libcamera::Camera> >)’ > 85 | for (auto [index, camera] : utils::enumerate(cm_->cameras())) { > > utils::enumerate() takes an lvalue reference, and cm_->cameras() is an > rvalue. I'll add the following to the documentation of the function: * Note that the argument to enumerate() has to be an lvalue, as the lifetime * of any rvalue would not be extended to the whole for loop. The compiler will * complain if an rvalue is passed to the function, in which case it should be * stored in a local variable before the loop. > > I probably really don't want to know - but why does it need this? > > https://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression > > cm_->cameras() returns a temporay value, which is passed to > utils::enumerate(). utils::enumerate() returns another temporary that > stores a reference to cm_->cameras(). The lifetime of the second > temporary is extended to the whole loop, but the lifetime of the first > temporary isn't. > > > It seems like an odd limitation, but if it's just a small corner case, > > then I don't mind having it split out. It helps keep line lengths > > shorter anyway...? > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Changes since v1: > > > > > > - Use structured bindings > > > --- > > > src/v4l2/v4l2_compat_manager.cpp | 11 +++++------ > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp > > > index 90c0f0121a32..96dbcdf28f04 100644 > > > --- a/src/v4l2/v4l2_compat_manager.cpp > > > +++ b/src/v4l2/v4l2_compat_manager.cpp > > > @@ -23,6 +23,7 @@ > > > #include <libcamera/camera_manager.h> > > > > > > #include "libcamera/internal/log.h" > > > +#include "libcamera/internal/utils.h" > > > > > > #include "v4l2_camera_file.h" > > > > > > @@ -81,11 +82,10 @@ int V4L2CompatManager::start() > > > * For each Camera registered in the system, a V4L2CameraProxy gets > > > * created here to wrap a camera device. > > > */ > > > - unsigned int index = 0; > > > - for (auto &camera : cm_->cameras()) { > > > + auto cameras = cm_->cameras(); > > > + for (auto [index, camera] : utils::enumerate(cameras)) { > > > V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); > > > proxies_.emplace_back(proxy); > > > - ++index; > > > } > > > > > > return 0; > > > @@ -117,11 +117,10 @@ int V4L2CompatManager::getCameraIndex(int fd) > > > if (!target) > > > return -1; > > > > > > - unsigned int index = 0; > > > - for (auto &camera : cm_->cameras()) { > > > + auto cameras = cm_->cameras(); > > > + for (auto [index, camera] : utils::enumerate(cameras)) { > > > if (camera == target) > > > return index; > > > - ++index; > > > } > > > > > > return -1;
Hi Laurent, On 18/05/2021 11:42, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, May 17, 2021 at 07:36:43PM +0300, Laurent Pinchart wrote: >> On Mon, May 17, 2021 at 01:54:08PM +0100, Kieran Bingham wrote: >>> On 15/05/2021 05:05, Laurent Pinchart wrote: >>>> Use the newly introduced utils::enumerate() to replace manual loop >>>> counters. A local variable is needed, as utils::enumerate() requires an >>>> lvalue reference. >>> >>> So this is not possible? >>> >>> for (auto [index, camera] : utils::enumerate(cm_->cameras()) { >> >> Unfortunately not :-( >> >>> I guess that's fine, but will it be obvious to someone trying to use it >>> that they need to move the thing to enumerate to a local variable if >>> it's not there? >> >> The compiler should make it obvious: >> >> ../../src/v4l2/v4l2_compat_manager.cpp: In member function ‘int V4L2CompatManager::start()’: >> ../../src/v4l2/v4l2_compat_manager.cpp:85:53: error: no matching function for call to ‘enumerate(std::vector<std::shared_ptr<libcamera::Camera> >)’ >> 85 | for (auto [index, camera] : utils::enumerate(cm_->cameras())) { >> >> utils::enumerate() takes an lvalue reference, and cm_->cameras() is an >> rvalue. > > I'll add the following to the documentation of the function: > > * Note that the argument to enumerate() has to be an lvalue, as the lifetime > * of any rvalue would not be extended to the whole for loop. The compiler will > * complain if an rvalue is passed to the function, in which case it should be > * stored in a local variable before the loop. Perfect.
diff --git a/src/v4l2/v4l2_compat_manager.cpp b/src/v4l2/v4l2_compat_manager.cpp index 90c0f0121a32..96dbcdf28f04 100644 --- a/src/v4l2/v4l2_compat_manager.cpp +++ b/src/v4l2/v4l2_compat_manager.cpp @@ -23,6 +23,7 @@ #include <libcamera/camera_manager.h> #include "libcamera/internal/log.h" +#include "libcamera/internal/utils.h" #include "v4l2_camera_file.h" @@ -81,11 +82,10 @@ int V4L2CompatManager::start() * For each Camera registered in the system, a V4L2CameraProxy gets * created here to wrap a camera device. */ - unsigned int index = 0; - for (auto &camera : cm_->cameras()) { + auto cameras = cm_->cameras(); + for (auto [index, camera] : utils::enumerate(cameras)) { V4L2CameraProxy *proxy = new V4L2CameraProxy(index, camera); proxies_.emplace_back(proxy); - ++index; } return 0; @@ -117,11 +117,10 @@ int V4L2CompatManager::getCameraIndex(int fd) if (!target) return -1; - unsigned int index = 0; - for (auto &camera : cm_->cameras()) { + auto cameras = cm_->cameras(); + for (auto [index, camera] : utils::enumerate(cameras)) { if (camera == target) return index; - ++index; } return -1;
Use the newly introduced utils::enumerate() to replace manual loop counters. A local variable is needed, as utils::enumerate() requires an lvalue reference. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Use structured bindings --- src/v4l2/v4l2_compat_manager.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)