[libcamera-devel] [PATCH v2 2/4] v4l2: Replace manual loop counters with utils::enumerate()

[Laurent Pinchart]

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 at ideasonboard.com>
> > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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;

-- 
Regards,

Laurent Pinchart