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

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2019-03-26 11:30:13 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Mar 26, 2019 at 12:47:35AM +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 instead of
> > setting values using the constructor a new add() method is used.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/cam/options.cpp | 19 +++++++++++++++++++
> >  src/cam/options.h   |  7 +++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > index 497833397d894f82..0dec154815d3cad5 100644
> > --- a/src/cam/options.cpp
> > +++ b/src/cam/options.cpp
> > @@ -272,6 +272,12 @@ OptionValue::OptionValue(const KeyValueParser::Options &value)
> >  {
> >  }
> >
> > +void OptionValue::add(const OptionValue &value)
> > +{
> > +	type_ = ValueArray;
> > +	array_.push_back(value);
> > +}
> > +
> 
> I wonder how that would look like if we separate OptionValue (which
> holds the actual multi-type option value) from the array.
> 
> I'm not expert of this code, but OptionBase has a 'values_' map, which
> associates the opt key with an OptionValue that holds the actually option
> value and this OptionValue, since this patch, could be an array too.
> 
> I wonder how that would look like it the 'values_' map would use
> another type, which maintains OptionValues into a a vector, so that
> all OptionValues could be stored as array without introducing the new
> 'array_' field.
> 
> Something like:
> 
> class OptionBase
> {
> 
>         ...
> 	std::map<T, OptionList> values_;
> 
> };
> 
> class OptionList
> {
>         ...
>         std::vector<OptionValue> array_;
> };
> 
> class OptionValue
> {
>         ....
>         Hold the basic types as it did already;
> };
> 
> Does this make any sense to you?

That would have been a nice idea, if all options where arrays. As array 
options are the exception, the main use-case is non-array options. Using 
your suggestion a user of the parser would have to jump thru hoops to 
access the non-array options by accessing them in a vector with 1 
member, right?

> 
> Thanks
>   j
> 
> >  OptionValue::operator int() const
> >  {
> >  	return toInteger();
> > @@ -287,6 +293,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 +322,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..6a887416c0070c41 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();
> > @@ -92,21 +94,26 @@ public:
> >  	OptionValue(const std::string &value);
> >  	OptionValue(const KeyValueParser::Options &value);
> >
> > +	void add(const OptionValue &value);
> > +
> >  	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_;
> >  };
> >
> >  class OptionsParser
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund