[libcamera-devel] [PATCH 3/3] cam: options: Add support for repeatable options

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Mar 26, 2019 at 12:47:36AM +0100, Niklas Söderlund wrote:
> Add a flag to indicate if an option can be repeatable. If an option is
> repeatable it must be accessed thru the array interface, even if it's
> only specified once by the user.
> 
> Also update the usage generator to indicate that tan option is

s/tan/that/?

> repeatable.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/options.cpp | 21 +++++++++++++++------
>  src/cam/options.h   |  5 +++--
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 0dec154815d3cad5..0fdde9d84ba0de0e 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -96,7 +96,11 @@ bool OptionsBase<T>::parseValue(const T &opt, const Option &option,
>  		break;
>  	}
>  
> -	values_[opt] = value;
> +	if (option.array)
> +		values_[opt].add(value);
> +	else
> +		values_[opt] = value;

I would be tempted to overload the [] operator for values_ to just do
the right thing depending on the type, but I think it would be bother
overkill and confusing. Still tempting to play with the available toys
though :-)

> +
>  	return true;
>  }
>  
> @@ -128,7 +132,7 @@ bool KeyValueParser::addOption(const char *name, OptionType type,
>  		return false;
>  
>  	optionsMap_[name] = Option({ 0, type, name, argument, nullptr,
> -				     help, nullptr });
> +				     help, nullptr, false });
>  	return true;
>  }
>  
> @@ -336,7 +340,7 @@ std::vector<OptionValue> OptionValue::toArray() const
>  
>  bool OptionsParser::addOption(int opt, OptionType type, const char *help,
>  			      const char *name, OptionArgument argument,
> -			      const char *argumentName)
> +			      const char *argumentName, bool array)
>  {
>  	/*
>  	 * Options must have at least a short or long name, and a text message.
> @@ -354,16 +358,16 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help,
>  		return false;
>  
>  	options_.push_back(Option({ opt, type, name, argument, argumentName,
> -				    help, nullptr }));
> +				    help, nullptr, array }));
>  	optionsMap_[opt] = &options_.back();
>  	return true;
>  }
>  
>  bool OptionsParser::addOption(int opt, KeyValueParser *parser, const char *help,
> -			      const char *name)
> +			      const char *name, bool array)
>  {
>  	if (!addOption(opt, OptionKeyValue, help, name, ArgumentRequired,
> -		       "key=value[,key=value,...]"))
> +		       "key=value[,key=value,...]", array))
>  		return false;
>  
>  	options_.back().keyValueParser = parser;
> @@ -461,6 +465,8 @@ void OptionsParser::usage()
>  			length += 1 + strlen(option.argumentName);
>  		if (option.argument == ArgumentOptional)
>  			length += 2;
> +		if (option.array)
> +			length += 4;
>  
>  		if (length > indent)
>  			indent = length;
> @@ -494,6 +500,9 @@ void OptionsParser::usage()
>  				argument += "]";
>  		}
>  
> +		if (option.array)
> +			argument += " ...";
> +
>  		std::cerr << std::setw(indent) << std::left << argument;
>  
>  		for (const char *help = option.help, *end = help; end; ) {
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 6a887416c0070c41..1dac15ea90f2ffd2 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -36,6 +36,7 @@ struct Option {
>  	const char *argumentName;
>  	const char *help;
>  	KeyValueParser *keyValueParser;
> +	bool array;

Maybe isArray ?

I think the rest is fine. How does the help output look like ?

>  
>  	bool hasShortOption() const { return isalnum(opt); }
>  	bool hasLongOption() const { return name != nullptr; }
> @@ -126,9 +127,9 @@ public:
>  	bool addOption(int opt, OptionType type, const char *help,
>  		       const char *name = nullptr,
>  		       OptionArgument argument = ArgumentNone,
> -		       const char *argumentName = nullptr);
> +		       const char *argumentName = nullptr, bool array = false);
>  	bool addOption(int opt, KeyValueParser *parser, const char *help,
> -		       const char *name = nullptr);
> +		       const char *name = nullptr, bool array = false);
>  
>  	Options parse(int argc, char *argv[]);
>  	void usage();

-- 
Regards,

Laurent Pinchart