| Message ID | 20200427220529.1085074-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. s/invalided/invalidate/ in the subject line. On Tue, Apr 28, 2020 at 12:05:26AM +0200, Niklas Söderlund wrote: > Extend OptionsBase<T> with a public invalidate() method. This allows > sub-classes to continue the interpreting of parsed options and > invalidate them if they find problems with the result. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/options.cpp | 6 ++++++ > src/cam/options.h | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > index 2c56eacf006e9857..77b3cc1f8c5a59e9 100644 > --- a/src/cam/options.cpp > +++ b/src/cam/options.cpp > @@ -64,6 +64,12 @@ const OptionValue &OptionsBase<T>::operator[](const T &opt) const > return values_.find(opt)->second; > } > > +template<typename T> > +void OptionsBase<T>::invalidate() > +{ > + valid_ = false; > +} > + > template<typename T> > bool OptionsBase<T>::parseValue(const T &opt, const Option &option, > const char *optarg) > diff --git a/src/cam/options.h b/src/cam/options.h > index 5060fee0c26b896f..8ccbfee48f7ca0b5 100644 > --- a/src/cam/options.h > +++ b/src/cam/options.h > @@ -54,6 +54,8 @@ public: > bool isSet(const T &opt) const; > const OptionValue &operator[](const T &opt) const; > > + void invalidate(); If this is only for subclasses, the method should be protected. I'm tempted to go simpler and move the valid_ field protected. What do you think ? If you want to keep the method I'd make it inline, as it will be more efficient. > + > private: > friend class KeyValueParser; > friend class OptionsParser;
Hi Laurent, Thanks for your feedback. On 2020-04-28 01:53:40 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > s/invalided/invalidate/ in the subject line. > > On Tue, Apr 28, 2020 at 12:05:26AM +0200, Niklas Söderlund wrote: > > Extend OptionsBase<T> with a public invalidate() method. This allows > > sub-classes to continue the interpreting of parsed options and > > invalidate them if they find problems with the result. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/options.cpp | 6 ++++++ > > src/cam/options.h | 2 ++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > > index 2c56eacf006e9857..77b3cc1f8c5a59e9 100644 > > --- a/src/cam/options.cpp > > +++ b/src/cam/options.cpp > > @@ -64,6 +64,12 @@ const OptionValue &OptionsBase<T>::operator[](const T &opt) const > > return values_.find(opt)->second; > > } > > > > +template<typename T> > > +void OptionsBase<T>::invalidate() > > +{ > > + valid_ = false; > > +} > > + > > template<typename T> > > bool OptionsBase<T>::parseValue(const T &opt, const Option &option, > > const char *optarg) > > diff --git a/src/cam/options.h b/src/cam/options.h > > index 5060fee0c26b896f..8ccbfee48f7ca0b5 100644 > > --- a/src/cam/options.h > > +++ b/src/cam/options.h > > @@ -54,6 +54,8 @@ public: > > bool isSet(const T &opt) const; > > const OptionValue &operator[](const T &opt) const; > > > > + void invalidate(); > > If this is only for subclasses, the method should be protected. I'm > tempted to go simpler and move the valid_ field protected. What do you > think ? If you want to keep the method I'd make it inline, as it will be > more efficient. My bad the commit message is wrong, it's not for subclassing. It's for processing the KeyValueParser::Options (which inherits from OptionsBase<T>) in subclasses of KeyValueParser. One could move valid_ to protected and the implement KeyValueParser::Options::invalidate() to hide it a bit more. But I don't see the harm in allowing even applications to examine the options parsed from the command line and if it does not like them marking the whole set as invalid. I would not however expose a setValid(bool valid) interface to applications. I have no strong feelings here so I'm open to any solution tho. > > > + > > private: > > friend class KeyValueParser; > > friend class OptionsParser; > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Tue, Apr 28, 2020 at 01:13:52AM +0200, Niklas Söderlund wrote: > On 2020-04-28 01:53:40 +0300, Laurent Pinchart wrote: > > > > s/invalided/invalidate/ in the subject line. > > > > On Tue, Apr 28, 2020 at 12:05:26AM +0200, Niklas Söderlund wrote: > > > Extend OptionsBase<T> with a public invalidate() method. This allows > > > sub-classes to continue the interpreting of parsed options and > > > invalidate them if they find problems with the result. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/cam/options.cpp | 6 ++++++ > > > src/cam/options.h | 2 ++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > > > index 2c56eacf006e9857..77b3cc1f8c5a59e9 100644 > > > --- a/src/cam/options.cpp > > > +++ b/src/cam/options.cpp > > > @@ -64,6 +64,12 @@ const OptionValue &OptionsBase<T>::operator[](const T &opt) const > > > return values_.find(opt)->second; > > > } > > > > > > +template<typename T> > > > +void OptionsBase<T>::invalidate() > > > +{ > > > + valid_ = false; > > > +} > > > + > > > template<typename T> > > > bool OptionsBase<T>::parseValue(const T &opt, const Option &option, > > > const char *optarg) > > > diff --git a/src/cam/options.h b/src/cam/options.h > > > index 5060fee0c26b896f..8ccbfee48f7ca0b5 100644 > > > --- a/src/cam/options.h > > > +++ b/src/cam/options.h > > > @@ -54,6 +54,8 @@ public: > > > bool isSet(const T &opt) const; > > > const OptionValue &operator[](const T &opt) const; > > > > > > + void invalidate(); > > > > If this is only for subclasses, the method should be protected. I'm > > tempted to go simpler and move the valid_ field protected. What do you > > think ? If you want to keep the method I'd make it inline, as it will be > > more efficient. > > My bad the commit message is wrong, it's not for subclassing. It's for > processing the KeyValueParser::Options (which inherits from > OptionsBase<T>) in subclasses of KeyValueParser. > > One could move valid_ to protected and the implement > KeyValueParser::Options::invalidate() to hide it a bit more. But I don't > see the harm in allowing even applications to examine the options parsed > from the command line and if it does not like them marking the whole set > as invalid. I would not however expose a setValid(bool valid) interface > to applications. > > I have no strong feelings here so I'm open to any solution tho. It's an internal API, doesn't matter too much. With the commit message updated, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > > private: > > > friend class KeyValueParser; > > > friend class OptionsParser;
diff --git a/src/cam/options.cpp b/src/cam/options.cpp index 2c56eacf006e9857..77b3cc1f8c5a59e9 100644 --- a/src/cam/options.cpp +++ b/src/cam/options.cpp @@ -64,6 +64,12 @@ const OptionValue &OptionsBase<T>::operator[](const T &opt) const return values_.find(opt)->second; } +template<typename T> +void OptionsBase<T>::invalidate() +{ + valid_ = false; +} + template<typename T> bool OptionsBase<T>::parseValue(const T &opt, const Option &option, const char *optarg) diff --git a/src/cam/options.h b/src/cam/options.h index 5060fee0c26b896f..8ccbfee48f7ca0b5 100644 --- a/src/cam/options.h +++ b/src/cam/options.h @@ -54,6 +54,8 @@ public: bool isSet(const T &opt) const; const OptionValue &operator[](const T &opt) const; + void invalidate(); + private: friend class KeyValueParser; friend class OptionsParser;
Extend OptionsBase<T> with a public invalidate() method. This allows sub-classes to continue the interpreting of parsed options and invalidate them if they find problems with the result. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/options.cpp | 6 ++++++ src/cam/options.h | 2 ++ 2 files changed, 8 insertions(+)