[v2,1/5] libcamera: controls: Give name to the union containing storage
diff mbox series

Message ID 20250421153556.171192-2-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: controls: Move constructor/assignment + swap
Related show

Commit Message

Barnabás Pőcze April 21, 2025, 3:35 p.m. UTC 
In order to be able to copy the storage as one unit, regardless of
which member is active give a name to the union member.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 include/libcamera/controls.h |  6 +++---
 src/libcamera/controls.cpp   | 17 ++++++++---------
 2 files changed, 11 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Sept. 1, 2026, 12:10 p.m. UTC | #1 
On Mon, Apr 21, 2025 at 05:35:52PM +0200, Barnabás Pőcze wrote:
> In order to be able to copy the storage as one unit, regardless of
> which member is active give a name to the union member.

The patch does more than that, it also changes the name of the union
members, uses std::exchange(), and drops a reinterpret_cast. I'm fine
with the changes, only the commit message could do with improvements.

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

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  include/libcamera/controls.h |  6 +++---
>  src/libcamera/controls.cpp   | 17 ++++++++---------
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 4bfe9615c..1dc6ccffa 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -238,9 +238,9 @@ private:
>  	bool isArray_;
>  	std::size_t numElements_ : 32;
>  	union {
> -		uint64_t value_;
> -		void *storage_;
> -	};
> +		uint64_t internal;
> +		void *external;
> +	} storage_;
>  
>  	void release();
>  	void set(ControlType type, bool isArray, const void *data,
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 70f6f6092..d384e1ef7 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -10,6 +10,7 @@
>  #include <sstream>
>  #include <string.h>
>  #include <string>
> +#include <utility>
>  
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
> @@ -122,10 +123,8 @@ void ControlValue::release()
>  {
>  	std::size_t size = numElements_ * ControlValueSize[type_];
>  
> -	if (size > sizeof(value_)) {
> -		delete[] reinterpret_cast<uint8_t *>(storage_);
> -		storage_ = nullptr;
> -	}
> +	if (size > sizeof(storage_.internal))
> +		delete[] reinterpret_cast<uint8_t *>(std::exchange(storage_.external, nullptr));
>  }
>  
>  ControlValue::~ControlValue()
> @@ -192,9 +191,9 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
>  Span<const uint8_t> ControlValue::data() const
>  {
>  	std::size_t size = numElements_ * ControlValueSize[type_];
> -	const uint8_t *data = size > sizeof(value_)
> -			    ? reinterpret_cast<const uint8_t *>(storage_)
> -			    : reinterpret_cast<const uint8_t *>(&value_);
> +	const uint8_t *data = size > sizeof(storage_.internal)
> +			    ? reinterpret_cast<const uint8_t *>(storage_.external)
> +			    : reinterpret_cast<const uint8_t *>(&storage_.internal);
>  	return { data, size };
>  }
>  
> @@ -391,8 +390,8 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>  	if (oldSize == newSize)
>  		return;
>  
> -	if (newSize > sizeof(value_))
> -		storage_ = reinterpret_cast<void *>(new uint8_t[newSize]);
> +	if (newSize > sizeof(storage_.internal))
> +		storage_.external = new uint8_t[newSize];
>  }
>  
>  /**
Laurent Pinchart Sept. 1, 2026, 12:11 p.m. UTC | #2 
On Tue, Sep 01, 2026 at 03:10:54PM +0300, Laurent Pinchart wrote:
> On Mon, Apr 21, 2025 at 05:35:52PM +0200, Barnabás Pőcze wrote:
> > In order to be able to copy the storage as one unit, regardless of
> > which member is active give a name to the union member.
> 
> The patch does more than that, it also changes the name of the union
> members, uses std::exchange(), and drops a reinterpret_cast. I'm fine
> with the changes, only the commit message could do with improvements.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  include/libcamera/controls.h |  6 +++---
> >  src/libcamera/controls.cpp   | 17 ++++++++---------
> >  2 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 4bfe9615c..1dc6ccffa 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -238,9 +238,9 @@ private:
> >  	bool isArray_;
> >  	std::size_t numElements_ : 32;
> >  	union {
> > -		uint64_t value_;
> > -		void *storage_;
> > -	};
> > +		uint64_t internal;
> > +		void *external;
> > +	} storage_;
> >  
> >  	void release();
> >  	void set(ControlType type, bool isArray, const void *data,
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 70f6f6092..d384e1ef7 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -10,6 +10,7 @@
> >  #include <sstream>
> >  #include <string.h>
> >  #include <string>
> > +#include <utility>
> >  
> >  #include <libcamera/base/log.h>
> >  #include <libcamera/base/utils.h>
> > @@ -122,10 +123,8 @@ void ControlValue::release()
> >  {
> >  	std::size_t size = numElements_ * ControlValueSize[type_];
> >  
> > -	if (size > sizeof(value_)) {
> > -		delete[] reinterpret_cast<uint8_t *>(storage_);
> > -		storage_ = nullptr;
> > -	}
> > +	if (size > sizeof(storage_.internal))
> > +		delete[] reinterpret_cast<uint8_t *>(std::exchange(storage_.external, nullptr));

Actually, I think the original code is easier to read. I'd drop the
change (the rename should stay of course).

> >  }
> >  
> >  ControlValue::~ControlValue()
> > @@ -192,9 +191,9 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
> >  Span<const uint8_t> ControlValue::data() const
> >  {
> >  	std::size_t size = numElements_ * ControlValueSize[type_];
> > -	const uint8_t *data = size > sizeof(value_)
> > -			    ? reinterpret_cast<const uint8_t *>(storage_)
> > -			    : reinterpret_cast<const uint8_t *>(&value_);
> > +	const uint8_t *data = size > sizeof(storage_.internal)
> > +			    ? reinterpret_cast<const uint8_t *>(storage_.external)
> > +			    : reinterpret_cast<const uint8_t *>(&storage_.internal);
> >  	return { data, size };
> >  }
> >  
> > @@ -391,8 +390,8 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
> >  	if (oldSize == newSize)
> >  		return;
> >  
> > -	if (newSize > sizeof(value_))
> > -		storage_ = reinterpret_cast<void *>(new uint8_t[newSize]);
> > +	if (newSize > sizeof(storage_.internal))
> > +		storage_.external = new uint8_t[newSize];
> >  }
> >  
> >  /**
Barnabás Pőcze Sept. 1, 2026, 1:17 p.m. UTC | #3 
2026. 09. 01. 14:11 keltezéssel, Laurent Pinchart írta:
> On Tue, Sep 01, 2026 at 03:10:54PM +0300, Laurent Pinchart wrote:
>> On Mon, Apr 21, 2025 at 05:35:52PM +0200, Barnabás Pőcze wrote:
>>> In order to be able to copy the storage as one unit, regardless of
>>> which member is active give a name to the union member.
>>
>> The patch does more than that, it also changes the name of the union
>> members, uses std::exchange(), and drops a reinterpret_cast. I'm fine
>> with the changes, only the commit message could do with improvements.

Done.


>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>>   include/libcamera/controls.h |  6 +++---
>>>   src/libcamera/controls.cpp   | 17 ++++++++---------
>>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index 4bfe9615c..1dc6ccffa 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -238,9 +238,9 @@ private:
>>>   	bool isArray_;
>>>   	std::size_t numElements_ : 32;
>>>   	union {
>>> -		uint64_t value_;
>>> -		void *storage_;
>>> -	};
>>> +		uint64_t internal;
>>> +		void *external;
>>> +	} storage_;
>>>   
>>>   	void release();
>>>   	void set(ControlType type, bool isArray, const void *data,
>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>>> index 70f6f6092..d384e1ef7 100644
>>> --- a/src/libcamera/controls.cpp
>>> +++ b/src/libcamera/controls.cpp
>>> @@ -10,6 +10,7 @@
>>>   #include <sstream>
>>>   #include <string.h>
>>>   #include <string>
>>> +#include <utility>
>>>   
>>>   #include <libcamera/base/log.h>
>>>   #include <libcamera/base/utils.h>
>>> @@ -122,10 +123,8 @@ void ControlValue::release()
>>>   {
>>>   	std::size_t size = numElements_ * ControlValueSize[type_];
>>>   
>>> -	if (size > sizeof(value_)) {
>>> -		delete[] reinterpret_cast<uint8_t *>(storage_);
>>> -		storage_ = nullptr;
>>> -	}
>>> +	if (size > sizeof(storage_.internal))
>>> +		delete[] reinterpret_cast<uint8_t *>(std::exchange(storage_.external, nullptr));
> 
> Actually, I think the original code is easier to read. I'd drop the
> change (the rename should stay of course).

Done.


> 
>>>   }
>>>   
>>>   ControlValue::~ControlValue()
>>> @@ -192,9 +191,9 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
>>>   Span<const uint8_t> ControlValue::data() const
>>>   {
>>>   	std::size_t size = numElements_ * ControlValueSize[type_];
>>> -	const uint8_t *data = size > sizeof(value_)
>>> -			    ? reinterpret_cast<const uint8_t *>(storage_)
>>> -			    : reinterpret_cast<const uint8_t *>(&value_);
>>> +	const uint8_t *data = size > sizeof(storage_.internal)
>>> +			    ? reinterpret_cast<const uint8_t *>(storage_.external)
>>> +			    : reinterpret_cast<const uint8_t *>(&storage_.internal);
>>>   	return { data, size };
>>>   }
>>>   
>>> @@ -391,8 +390,8 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>>>   	if (oldSize == newSize)
>>>   		return;
>>>   
>>> -	if (newSize > sizeof(value_))
>>> -		storage_ = reinterpret_cast<void *>(new uint8_t[newSize]);
>>> +	if (newSize > sizeof(storage_.internal))
>>> +		storage_.external = new uint8_t[newSize];
>>>   }
>>>   
>>>   /**
>

Patch
diff mbox series 

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 4bfe9615c..1dc6ccffa 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -238,9 +238,9 @@  private:
 	bool isArray_;
 	std::size_t numElements_ : 32;
 	union {
-		uint64_t value_;
-		void *storage_;
-	};
+		uint64_t internal;
+		void *external;
+	} storage_;
 
 	void release();
 	void set(ControlType type, bool isArray, const void *data,
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
index 70f6f6092..d384e1ef7 100644
--- a/src/libcamera/controls.cpp
+++ b/src/libcamera/controls.cpp
@@ -10,6 +10,7 @@ 
 #include <sstream>
 #include <string.h>
 #include <string>
+#include <utility>
 
 #include <libcamera/base/log.h>
 #include <libcamera/base/utils.h>
@@ -122,10 +123,8 @@  void ControlValue::release()
 {
 	std::size_t size = numElements_ * ControlValueSize[type_];
 
-	if (size > sizeof(value_)) {
-		delete[] reinterpret_cast<uint8_t *>(storage_);
-		storage_ = nullptr;
-	}
+	if (size > sizeof(storage_.internal))
+		delete[] reinterpret_cast<uint8_t *>(std::exchange(storage_.external, nullptr));
 }
 
 ControlValue::~ControlValue()
@@ -192,9 +191,9 @@  ControlValue &ControlValue::operator=(const ControlValue &other)
 Span<const uint8_t> ControlValue::data() const
 {
 	std::size_t size = numElements_ * ControlValueSize[type_];
-	const uint8_t *data = size > sizeof(value_)
-			    ? reinterpret_cast<const uint8_t *>(storage_)
-			    : reinterpret_cast<const uint8_t *>(&value_);
+	const uint8_t *data = size > sizeof(storage_.internal)
+			    ? reinterpret_cast<const uint8_t *>(storage_.external)
+			    : reinterpret_cast<const uint8_t *>(&storage_.internal);
 	return { data, size };
 }
 
@@ -391,8 +390,8 @@  void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
 	if (oldSize == newSize)
 		return;
 
-	if (newSize > sizeof(value_))
-		storage_ = reinterpret_cast<void *>(new uint8_t[newSize]);
+	if (newSize > sizeof(storage_.internal))
+		storage_.external = new uint8_t[newSize];
 }
 
 /**