[libcamera-devel] [PATCH 1/2] cam: Separate options valid() and empty()

[Laurent Pinchart]

Hi Jacopo,

On Tue, Mar 26, 2019 at 11:50:53AM +0100, Jacopo Mondi wrote:
> On Sat, Mar 23, 2019 at 09:31:24AM +0200, Laurent Pinchart wrote:
> > An empty option list is not necessarily an error. Add a new empty()
> > function to test the option list for emptiness, and modify the valid()
> > function to only notify parsing errors. As a side effect this allows
> > accessing partially parsed options, which may be useful in the future.
> >
> 
> Not an expert of this part, but this look sane to me apart from the
> fact I don't get where it is used? Do you need partial options in 2/2 ?

The qcam application doesn't require options, they're all optional, so I
need to be able to distinguish between invalid options and no option.

> On the single patch:
> (Not-sure-I-got-this-but)Acked-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/cam/main.cpp    |  7 +++++--
> >  src/cam/options.cpp | 34 +++++++++++++++-------------------
> >  src/cam/options.h   |  5 ++++-
> >  3 files changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 1ca7862bf237..e7490c32f99a 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -67,9 +67,12 @@ static int parseOptions(int argc, char *argv[])
> >  	parser.addOption(OptList, OptionNone, "List all cameras", "list");
> >
> >  	options = parser.parse(argc, argv);
> > -	if (!options.valid() || options.isSet(OptHelp)) {
> > +	if (!options.valid())
> > +		return -EINVAL;
> > +
> > +	if (options.empty() || options.isSet(OptHelp)) {
> >  		parser.usage();
> > -		return !options.valid() ? -EINVAL : -EINTR;
> > +		return options.empty() ? -EINVAL : -EINTR;
> >  	}
> >
> >  	return 0;
> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > index 655aa36bb9c9..f053a31d6ea1 100644
> > --- a/src/cam/options.cpp
> > +++ b/src/cam/options.cpp
> > @@ -39,10 +39,16 @@ const char *Option::typeName() const
> >   * OptionBase<T>
> >   */
> >
> > +template<typename T>
> > +bool OptionsBase<T>::empty() const
> > +{
> > +	return values_.empty();
> > +}
> > +
> >  template<typename T>
> >  bool OptionsBase<T>::valid() const
> >  {
> > -	return !values_.empty();
> > +	return valid_;
> >  }
> >
> >  template<typename T>
> > @@ -100,12 +106,6 @@ bool OptionsBase<T>::parseValue(const T &opt, const Option &option,
> >  	return true;
> >  }
> >
> > -template<typename T>
> > -void OptionsBase<T>::clear()
> > -{
> > -	values_.clear();
> > -}
> > -
> >  template class OptionsBase<int>;
> >  template class OptionsBase<std::string>;
> >
> > @@ -165,21 +165,18 @@ KeyValueParser::Options KeyValueParser::parse(const char *arguments)
> >
> >  		if (optionsMap_.find(key) == optionsMap_.end()) {
> >  			std::cerr << "Invalid option " << key << std::endl;
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		}
> >
> >  		OptionArgument arg = optionsMap_[key].argument;
> >  		if (value.empty() && arg == ArgumentRequired) {
> >  			std::cerr << "Option " << key << " requires an argument"
> >  				  << std::endl;
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		} else if (!value.empty() && arg == ArgumentNone) {
> >  			std::cerr << "Option " << key << " takes no argument"
> >  				  << std::endl;
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		}
> >
> >  		const Option &option = optionsMap_[key];
> > @@ -187,11 +184,11 @@ KeyValueParser::Options KeyValueParser::parse(const char *arguments)
> >  			std::cerr << "Failed to parse '" << value << "' as "
> >  				  << option.typeName() << " for option " << key
> >  				  << std::endl;
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		}
> >  	}
> >
> > +	options.valid_ = true;
> >  	return options;
> >  }
> >
> > @@ -412,19 +409,18 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
> >  			std::cerr << argv[optind - 1] << std::endl;
> >
> >  			usage();
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		}
> >
> >  		const Option &option = *optionsMap_[c];
> >  		if (!options.parseValue(c, option, optarg)) {
> >  			parseValueError(option);
> >  			usage();
> > -			options.clear();
> > -			break;
> > +			return options;
> >  		}
> >  	}
> >
> > +	options.valid_ = true;
> >  	return options;
> >  }
> >
> > diff --git a/src/cam/options.h b/src/cam/options.h
> > index 745f4a4a3a43..0b0444c2db42 100644
> > --- a/src/cam/options.h
> > +++ b/src/cam/options.h
> > @@ -45,6 +45,9 @@ template<typename T>
> >  class OptionsBase
> >  {
> >  public:
> > +	OptionsBase() : valid_(false) {}
> > +
> > +	bool empty() const;
> >  	bool valid() const;
> >  	bool isSet(const T &opt) const;
> >  	const OptionValue &operator[](const T &opt) const;
> > @@ -54,9 +57,9 @@ private:
> >  	friend class OptionsParser;
> >
> >  	bool parseValue(const T &opt, const Option &option, const char *value);
> > -	void clear();
> >
> >  	std::map<T, OptionValue> values_;
> > +	bool valid_;
> >  };
> >
> >  class KeyValueParser

-- 
Regards,

Laurent Pinchart