[libcamera-devel,1/2] cam: Separate options valid() and empty()
diff mbox series

Message ID 20190323073125.25497-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Qt-based libcamera viewer
Related show

Commit Message

Laurent Pinchart March 23, 2019, 7:31 a.m. UTC 
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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/main.cpp    |  7 +++++--
 src/cam/options.cpp | 34 +++++++++++++++-------------------
 src/cam/options.h   |  5 ++++-
 3 files changed, 24 insertions(+), 22 deletions(-)

Comments

Jacopo Mondi March 26, 2019, 10:50 a.m. UTC | #1 
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@jmondi.org>

Thanks
  j

> Signed-off-by: Laurent Pinchart <laurent.pinchart@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 26, 2019, 11:54 p.m. UTC | #2 
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@jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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
Niklas Söderlund March 27, 2019, 12:39 a.m. UTC | #3 
Hi Laurent,

Thanks for your work.

On 2019-03-23 09:31:24 +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.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Nice idea,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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