[libcamera-devel] [RFC 2/4] cam: options: Add an array data type to OptionValue

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Mar 22, 2019 at 02:53:47AM +0100, Niklas Söderlund wrote:
> To allow specifying the same argument option multiple times a new type
> of OptionValue is needed. As parsing of options is an iterative process
> there is a need to append options as they are parsed so a more complex
> constructor us needed which can combine already stored instances of an

s/us needed/is needed/

> option with new ones.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/options.cpp | 21 +++++++++++++++++++++
>  src/cam/options.h   |  6 ++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 497833397d894f82..7995a9b359764ec7 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -272,6 +272,14 @@ OptionValue::OptionValue(const KeyValueParser::Options &value)
>  {
>  }
>  
> +OptionValue::OptionValue(const OptionValue &value,
> +			 const std::vector<OptionValue> &array)
> +	: type_(ValueArray)
> +{
> +	array_ = array;
> +	array_.push_back(value);
> +}
> +
>  OptionValue::operator int() const
>  {
>  	return toInteger();
> @@ -287,6 +295,11 @@ OptionValue::operator KeyValueParser::Options() const
>  	return toKeyValues();
>  }
>  
> +OptionValue::operator std::vector<OptionValue>() const
> +{
> +	return toArray();
> +}
> +
>  int OptionValue::toInteger() const
>  {
>  	if (type_ != ValueInteger)
> @@ -311,6 +324,14 @@ KeyValueParser::Options OptionValue::toKeyValues() const
>  	return keyValues_;
>  }
>  
> +std::vector<OptionValue> OptionValue::toArray() const
> +{
> +	if (type_ != ValueArray)
> +		return std::vector<OptionValue>{};
> +
> +	return array_;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * OptionsParser
>   */
> diff --git a/src/cam/options.h b/src/cam/options.h
> index b33a90fc6058febf..789ba36187dd1fc3 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -10,6 +10,7 @@
>  #include <ctype.h>
>  #include <list>
>  #include <map>
> +#include <vector>
>  
>  class KeyValueParser;
>  class OptionValue;
> @@ -84,6 +85,7 @@ public:
>  		ValueInteger,
>  		ValueString,
>  		ValueKeyValue,
> +		ValueArray,
>  	};
>  
>  	OptionValue();
> @@ -91,22 +93,26 @@ public:
>  	OptionValue(const char *value);
>  	OptionValue(const std::string &value);
>  	OptionValue(const KeyValueParser::Options &value);
> +	OptionValue(const OptionValue &value, const std::vector<OptionValue> &array);

The patch looks fine except for this constructor. Isn't it possible for
the parser to create an OptionValue with an empty array the first time,
and add new values to the array of the same OptionvAlue, instead of
recreating a new OptionValue every time ?

>  
>  	ValueType type() const { return type_; }
>  
>  	operator int() const;
>  	operator std::string() const;
>  	operator KeyValueParser::Options() const;
> +	operator std::vector<OptionValue>() const;
>  
>  	int toInteger() const;
>  	std::string toString() const;
>  	KeyValueParser::Options toKeyValues() const;
> +	std::vector<OptionValue> toArray() const;
>  
>  private:
>  	ValueType type_;
>  	int integer_;
>  	std::string string_;
>  	KeyValueParser::Options keyValues_;
> +	std::vector<OptionValue> array_;

Should we group those four fields in a union ?

>  };
>  
>  class OptionsParser

-- 
Regards,

Laurent Pinchart