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

Message ID 20190322015349.14934-3-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • cam: Extend to support configuration of multiple streams
Related show

Commit Message

Niklas Söderlund March 22, 2019, 1:53 a.m. UTC 
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
option with new ones.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/options.cpp | 21 +++++++++++++++++++++
 src/cam/options.h   |  6 ++++++
 2 files changed, 27 insertions(+)

Comments

Laurent Pinchart March 23, 2019, 2:28 p.m. UTC | #1 
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@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
Niklas Söderlund March 25, 2019, 10:18 p.m. UTC | #2 
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@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

Patch
diff mbox series 

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);
 
 	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