[libcamera-devel,2/6] cam: options: create a template class for options

423 diff mbox series

In preparation to adding more parsers create a template class to hold
the parsed information. The rational for making it a template are that
different parsers can index the options using different data types.

The OptionsParser index its options using an int while the upcoming
KeyValyeParser will index its options using strings for example.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/options.cpp | 24 ------------------------
 src/cam/options.h   | 28 ++++++++++++++++------------
 2 files changed, 16 insertions(+), 36 deletions(-)

Hi Niklas,

Thank you for the patch.

On Mon, Jan 28, 2019 at 01:41:05AM +0100, Niklas Söderlund wrote:
> In preparation to adding more parsers create a template class to hold
> the parsed information. The rational for making it a template are that
> different parsers can index the options using different data types.
> 
> The OptionsParser index its options using an int while the upcoming
> KeyValyeParser will index its options using strings for example.

s/Valye/Value/

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/options.cpp | 24 ------------------------
>  src/cam/options.h   | 28 ++++++++++++++++------------
>  2 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 83601270207b67b3..b24964a8ce413a85 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -166,27 +166,3 @@ void OptionsParser::usage()
>  		}
>  	}
>  }
> -
> -OptionsParser::Options::Options()
> -{
> -}
> -
> -bool OptionsParser::Options::valid() const
> -{
> -	return !values_.empty();
> -}
> -
> -bool OptionsParser::Options::isSet(int opt) const
> -{
> -	return values_.find(opt) != values_.end();
> -}
> -
> -const std::string &OptionsParser::Options::operator[](int opt) const
> -{
> -	return values_.find(opt)->second;
> -}
> -
> -void OptionsParser::Options::clear()
> -{
> -	values_.clear();
> -}
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 491f6a316fffbe5b..a08bfea1ba74c96b 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -17,21 +17,25 @@ enum OptionArgument {
>  	ArgumentOptional,
>  };
>  
> +template <class T>
> +class OptionsBase
> +{
> +public:
> +	bool valid() const { return !values_.empty(); };
> +	bool isSet(T opt) const { return values_.find(opt) != values_.end(); };
> +	const std::string &operator[](T opt) const { return values_.find(opt)->second; };
> +
> +private:
> +	friend class OptionsParser;
> +	std::map<T, std::string> values_;
> +	void clear() { values_.clear(); };
> +};

One common issue with templates is how they push for inlining methods.
This is however not required when all the instantiations of the template
class are known in advance. In this specific case, you can for instance
apply the following change on top of this patch to avoid inlining
methods:

diff --git a/src/cam/options.cpp b/src/cam/options.cpp
index f63fd3b495e2..3f4781ae96bb 100644
--- a/src/cam/options.cpp
+++ b/src/cam/options.cpp
@@ -12,6 +12,33 @@

 #include "options.h"

+template <class T>
+bool OptionsBase<T>::valid() const
+{
+	return !values_.empty();
+}
+
+template <class T>
+bool OptionsBase<T>::isSet(T opt) const
+{
+	return values_.find(opt) != values_.end();
+}
+
+template <class T>
+const std::string &OptionsBase<T>::operator[](T opt) const
+{
+	return values_.find(opt)->second;
+}
+
+template <class T>
+void OptionsBase<T>::clear()
+{
+	values_.clear();
+}
+
+template class OptionsBase<int>;
+template class OptionsBase<std::string>;
+
 bool OptionsParser::addOption(int opt, const char *help, const char *name,
 			      OptionArgument argument, const char *argumentName)
 {
diff --git a/src/cam/options.h b/src/cam/options.h
index 91a78406a601..dff0d4f4ca22 100644
--- a/src/cam/options.h
+++ b/src/cam/options.h
@@ -21,15 +21,15 @@ template <class T>
 class OptionsBase
 {
 public:
-	bool valid() const { return !values_.empty(); };
-	bool isSet(T opt) const { return values_.find(opt) != values_.end(); };
-	const std::string &operator[](T opt) const { return values_.find(opt)->second; };
+	bool valid() const;
+	bool isSet(T opt) const;
+	const std::string &operator[](T opt) const;

 private:
 	friend class OptionsParser;
 	friend class KeyValueParser;
 	std::map<T, std::string> values_;
-	void clear() { values_.clear(); };
+	void clear();
 };

 class KeyValueParser

This isn't mandatory but may help reducing compilation time and memory
footprint when the cam application will grow.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  class OptionsParser
>  {
>  public:
> -	class Options {
> -	public:
> -		Options();
> -
> -		bool valid() const;
> -		bool isSet(int opt) const;
> -		const std::string &operator[](int opt) const;
> -
> -	private:
> -		friend class OptionsParser;
> -		std::map<int, std::string> values_;
> -		void clear();
> +	class Options : public OptionsBase<int>
> +	{
>  	};
>  
>  	void addOption(int opt, const char *help, const char *name = nullptr,