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

[Jacopo Mondi]

Hi Laurent,

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 ?

On the single patch:
(Not-sure-I-got-this-but)Acked-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190326/60b6b40f/attachment.sig>