[libcamera-devel] [PATCH 2/3] cam: options: Add an array data type to OptionValue
Jacopo Mondi
jacopo at jmondi.org
Tue Mar 26 12:27:05 CET 2019
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 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?
> >
> > 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 at lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190326/e9147637/attachment-0001.sig>
More information about the libcamera-devel
mailing list