| Message ID | 20190325234736.12533-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. 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@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) I think add is a bit too generic as a name. I would name this addValue() or possibly push_back() if we want to stick with the STL semantics (I think I have a preference for the former though). > +{ > + type_ = ValueArray; I wonder if this should error out if type_ is different than ValueArray or ValueNone. I suppose the caller wouldn't check for this though, and it would be a bug in the option parser, so I suppose we could keep it as-is (possibly with an std::assert ?). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + array_.push_back(value); > +} > + > 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
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@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? 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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Tue, Mar 26, 2019 at 11:35:25AM +0100, Niklas Söderlund wrote: > 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@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? Yes, they would be vectors with 1 member. Which part of handling a single entry vector concerns you? Insertion at parsing time or access to the option values? Thanks j > > > > > 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@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2019-03-26 11:40:45 +0100, Jacopo Mondi wrote: > Hi Niklas, > > On Tue, Mar 26, 2019 at 11:35:25AM +0100, Niklas Söderlund wrote: > > 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@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? > > Yes, they would be vectors with 1 member. Which part of handling a > single entry vector concerns you? Insertion at parsing time or access > to the option values? None of the above. What concerns me is how the usage of the parser, right now and with the array additions uses still access non-array options as: std::string name = options[OptCamera]; If everything was vectors it would need to be something like std::string name = options[OptCamera].front(); Which is not a nice usage interface as the bulk of options would not be repeatable. > > Thanks > j > > > > > > > > > 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@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hi Niklas, On Tue, Mar 26, 2019 at 12:00:19PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > On 2019-03-26 11:40:45 +0100, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Tue, Mar 26, 2019 at 11:35:25AM +0100, Niklas Söderlund wrote: > > > 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@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? > > > > Yes, they would be vectors with 1 member. Which part of handling a > > single entry vector concerns you? Insertion at parsing time or access > > to the option values? > > None of the above. What concerns me is how the usage of the parser, > right now and with the array additions uses still access non-array > options as: > > std::string name = options[OptCamera]; > > If everything was vectors it would need to be something like > > std::string name = options[OptCamera].front(); > > Which is not a nice usage interface as the bulk of options would not be > repeatable. > I don't want to insist, as this was just a suggestion and I don't know this code well enough, but I think with overloading of operator[] that you already implement to access the OptionValues stored in OptionBase's 'values_' this could be hidden easily (ie looking at the size of the options array in the below proposed OptionList or whatever). If that OptionList would provide an overloaded operator[] too you could then access array type options with 'options[OptSomething][SuboptSomething]'. Just suggestions anyway, surely not blocking this series :) Thanks j > > > > Thanks > > j > > > > > > > > > > > > > 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@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > -- > Regards, > Niklas Söderlund
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); +} + 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
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@ragnatech.se> --- src/cam/options.cpp | 19 +++++++++++++++++++ src/cam/options.h | 7 +++++++ 2 files changed, 26 insertions(+)