[libcamera-devel] [PATCH v3 4/7] libcamera: V4L2BufferCache: Use the entry reference

[Laurent Pinchart]

Hi Niklas,

On Thu, Mar 05, 2020 at 09:08:20PM +0100, Niklas Söderlund wrote:
> On 2020-03-05 01:28:36 +0200, Laurent Pinchart wrote:
> > On Thu, Mar 05, 2020 at 12:22:43AM +0100, Niklas Söderlund wrote:
> > > Instead of looking up the index in the storage vector use the reference
> > > to it created at the beginning of the loop.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > 
> > This could have been fixed in the next patches, when modifying the code.
> > Are you trying to improve your patch count ? ;-)
> 
> If I wanted to inflate my patch count I know of more efficient ways to 
> do so then this ;-P
> 
> I write patches as I would like them to read them if I where a reviewer.
> 
> Fixing things around the real change in the patch is a red flag for me 
> when reviewing. I have to spend time reviewing such patches by 
> questioning myself as I cant see why the change is needed only later to 
> find out that that line is changed just because we are modifying things 
> in the area anyhow. So whenever I find my self starting to describe more 
> then one change in the commit message I try break it out to it's own 
> patch, even if it's trivial.

Hence the "While at it, ..." mentions in the commit message :-) Without
them I agree it can be confusing.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > > ---
> > >  src/libcamera/v4l2_videodevice.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index d88cb0bd0771e545..268de60bc7965f58 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -216,7 +216,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> > >  			use = index;
> > >  
> > >  		/* Try to find a cache hit by comparing the planes. */
> > > -		if (cache_[index] == buffer) {
> > > +		if (entry == buffer) {
> > >  			hit = true;
> > >  			use = index;
> > >  			break;

-- 
Regards,

Laurent Pinchart