[libcamera-devel] [PATCH 4/6] cam: options: remove OptionsParser::options_

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Jan 28, 2019 at 01:41:07AM +0100, Niklas Söderlund wrote:
> It's unsafe to keep a pointer to an object inside a vector if one keeps
> adding and modifying the vectors content.

Very good point.

> There are also little need to keep the two data structures around when
> one map can solve the problem. Remove the vector and update all loops
> to iterate over the map instead of the vector.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/options.cpp | 18 +++++++++++-------
>  src/cam/options.h   |  3 +--
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index c9ca017b4cf3fa3d..d3bff1cd897a5cfb 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -30,8 +30,8 @@ bool OptionsParser::addOption(int opt, const char *help, const char *name,
>  	if (optionsMap_.find(opt) != optionsMap_.end())
>  		return false;
>  
> -	options_.push_back(Option({ opt, name, argument, argumentName, help }));
> -	optionsMap_[opt] = &options_.back();
> +	optionsMap_[opt] = Option({ opt, name, argument, argumentName, help });
> +
>  	return true;
>  }
>  
> @@ -43,14 +43,16 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)
>  	 * Allocate short and long options arrays large enough to contain all
>  	 * options.
>  	 */
> -	char shortOptions[options_.size() * 3 + 2] = {};
> -	struct option longOptions[options_.size() + 1] = {};
> +	char shortOptions[optionsMap_.size() * 3 + 2] = {};
> +	struct option longOptions[optionsMap_.size() + 1] = {};
>  	unsigned int ids = 0;
>  	unsigned int idl = 0;
>  
>  	shortOptions[ids++] = ':';
>  
> -	for (const Option &option : options_) {
> +	for (auto const &iter : optionsMap_) {
> +		const Option &option = iter.second;
> +
>  		if (option.hasShortOption()) {
>  			shortOptions[ids++] = option.opt;
>  			if (option.argument != ArgumentNone)
> @@ -112,7 +114,8 @@ void OptionsParser::usage()
>  
>  	unsigned int indent = 0;
>  
> -	for (const Option &option : options_) {
> +	for (auto const &iter : optionsMap_) {
> +		const Option &option = iter.second;

This will result in options being sorted base on their value. Do we
really want this ?

An alternative would be to replace the std::vector options_ with an
std::list that will not invalidate iterators and references when
inserting an element at the end.

>  		unsigned int length = 14;
>  		if (option.hasLongOption())
>  			length += 2 + strlen(option.name);
> @@ -127,7 +130,8 @@ void OptionsParser::usage()
>  
>  	indent = (indent + 7) / 8 * 8;
>  
> -	for (const Option &option : options_) {
> +	for (auto const &iter : optionsMap_) {
> +		const Option &option = iter.second;
>  		std::string argument;
>  		if (option.hasShortOption())
>  			argument = std::string("  -")
> diff --git a/src/cam/options.h b/src/cam/options.h
> index dfb7fcc9f6fa3324..cb7286a0a8005579 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -57,8 +57,7 @@ private:
>  		bool hasLongOption() const { return name != nullptr; }
>  	};
>  
> -	std::vector<Option> options_;
> -	std::map<unsigned int, Option *> optionsMap_;
> +	std::map<unsigned int, Option> optionsMap_;

You can rename optionsMap_ to options_, it will simplify the patch a
bit.

>  };
>  
>  #endif /* __CAM_OPTIONS_H__ */

-- 
Regards,

Laurent Pinchart