[PATCH v1 1/3] libcamera: controls: Give name to the union containing storage

[Jacopo Mondi]

Hi Barnabás

On Thu, Apr 17, 2025 at 01:35:37PM +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.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at 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;

We could nitpick on names here, but I won't :)

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

> +	} 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];
>  }
>
>  /**
> --
> 2.49.0
>