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

[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 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.

It's an internal API, doesn't matter too much. With the commit message
updated,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > +
> > >  private:
> > >  	friend class KeyValueParser;
> > >  	friend class OptionsParser;

-- 
Regards,

Laurent Pinchart