[libcamera-devel] [PATCH v2 2/5] cam: options: Add public method to invalided options

[Niklas Söderlund]

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 at 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

-- 
Regards,
Niklas Söderlund