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

[Laurent Pinchart]

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