[libcamera-devel] [PATCH 06/10] libcamera: v4l2_videodevice: Prevent aliasing of V4L2BufferCache members

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Oct 13, 2020 at 04:12:37PM +0100, Kieran Bingham wrote:
> The members free, and lastUsed were not following the libcamera coding
> style, and were producing an aliased parameter on the construction.
> 
> Rename them to be marked as member variables with the _ postfix
> accordingly.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  4 ++--
>  src/libcamera/v4l2_videodevice.cpp            | 14 +++++++-------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 40ed87e17cfa..53f6a2d5515b 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -127,8 +127,8 @@ private:
>  
>  		bool operator==(const FrameBuffer &buffer) const;
>  
> -		bool free;
> -		uint64_t lastUsed;
> +		bool free_;
> +		uint64_t lastUsed_;
>  
>  	private:
>  		struct Plane {
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 508522ef42bb..96952b26c634 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -212,7 +212,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  	for (unsigned int index = 0; index < cache_.size(); index++) {
>  		const Entry &entry = cache_[index];
>  
> -		if (!entry.free)
> +		if (!entry.free_)
>  			continue;
>  
>  		/* Try to find a cache hit by comparing the planes. */
> @@ -222,9 +222,9 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  			break;
>  		}
>  
> -		if (entry.lastUsed < oldest) {
> +		if (entry.lastUsed_ < oldest) {
>  			use = index;
> -			oldest = entry.lastUsed;
> +			oldest = entry.lastUsed_;
>  		}
>  	}
>  
> @@ -248,16 +248,16 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  void V4L2BufferCache::put(unsigned int index)
>  {
>  	ASSERT(index < cache_.size());
> -	cache_[index].free = true;
> +	cache_[index].free_ = true;
>  }
>  
>  V4L2BufferCache::Entry::Entry()
> -	: free(true), lastUsed(0)
> +	: free_(true), lastUsed_(0)
>  {
>  }
>  
> -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> -	: free(free), lastUsed(lastUsed)
> +V4L2BufferCache::Entry::Entry(bool free, uint64_t age, const FrameBuffer &buffer)
> +	: free_(free), lastUsed_(age)

Any specific reason to rename the function parameter here, or is it a
leftover ? I assume the latter, so without the rename,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  {
>  	for (const FrameBuffer::Plane &plane : buffer.planes())
>  		planes_.emplace_back(plane);

-- 
Regards,

Laurent Pinchart