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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2019-03-23 16:28:08 +0200, Laurent Pinchart wrote:
> 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 ?


I tried both approaches and deemed both to have the same level of evil.  
Have no preference so will fix this and the comment in 3/4 in next 
version.

> 
> >  
> >  	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 ?

I tried this as well, but due to that the union will have members with 
non-trivial constructors a custom constructor would be needed [1]. I 
thought this was too much work for the option parser to make it wort 
while. Will keep it as is for next version, if you disagree please let 
me know and we can fix it up on top.

1. https://en.cppreference.com/w/cpp/language/union

> 
> >  };
> >  
> >  class OptionsParser
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund