[libcamera-devel,06/10] libcamera: v4l2_videodevice: Prevent aliasing of V4L2BufferCache members
diff mbox series

Message ID 20201013151241.3557005-7-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • Shadowed Variables
Related show

Commit Message

Kieran Bingham Oct. 13, 2020, 3:12 p.m. UTC 
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@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h |  4 ++--
 src/libcamera/v4l2_videodevice.cpp            | 14 +++++++-------
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Niklas Söderlund Oct. 14, 2020, 12:28 p.m. UTC | #1 
Hi Kieran,

On 2020-10-13 16:12:37 +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@ideasonboard.com>

Sorry about this one :-)

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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)
>  {
>  	for (const FrameBuffer::Plane &plane : buffer.planes())
>  		planes_.emplace_back(plane);
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 15, 2020, 1:38 p.m. UTC | #2 
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@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@ideasonboard.com>

>  {
>  	for (const FrameBuffer::Plane &plane : buffer.planes())
>  		planes_.emplace_back(plane);
Kieran Bingham Oct. 15, 2020, 1:58 p.m. UTC | #3 
Hi Laurent,

On 15/10/2020 14:38, Laurent Pinchart wrote:
> 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@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,

Good spot - indeed, this is a leftover from before I saw adding the _
was the correct way forwards.

I'll fix for v2.


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

Thanks.


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

Patch
diff mbox series 

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)
 {
 	for (const FrameBuffer::Plane &plane : buffer.planes())
 		planes_.emplace_back(plane);