[libcamera-devel] [RFC 3/4] cam: options: Add parsing of multiple instances of the same option

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Mar 22, 2019 at 02:53:48AM +0100, Niklas Söderlund wrote:
> Add the ability to allow storing multiple instances of the same option.

"Allow multiple instances of the same option."

And this actually just duplicates the subject line, so maybe you need
something else :-)

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/options.cpp | 12 ++++++------
>  src/cam/options.h   |  5 +++--
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 7995a9b359764ec7..e9dcd0c39cdc50ce 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -96,7 +96,7 @@ bool OptionsBase<T>::parseValue(const T &opt, const Option &option,
>  		break;
>  	}
>  
> -	values_[opt] = value;
> +	values_[opt] = option.array ? OptionValue(value, values_[opt]) : value;

This is the part that bothers me, as explained in the review of 2/4.

You're also missing an update for the help text generation, options that
accept multiple instances should be marked as such.

Apart from that the patch looks good to me.

>  	return true;
>  }
>  
> @@ -128,7 +128,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;
>  }
>  
> @@ -338,7 +338,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.
> @@ -356,16 +356,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;
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 789ba36187dd1fc3..922db4650b49117d 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;
>  
>  	bool hasShortOption() const { return isalnum(opt); }
>  	bool hasLongOption() const { return name != nullptr; }
> @@ -125,9 +126,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