[{"id":719,"web_url":"https://patchwork.libcamera.org/comment/719/","msgid":"<20190131101340.GH4197@pendragon.ideasonboard.com>","date":"2019-01-31T10:13:40","subject":"Re: [libcamera-devel] [PATCH 4/6] cam: options: remove\n\tOptionsParser::options_","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Jan 28, 2019 at 01:41:07AM +0100, Niklas Söderlund wrote:\n> It's unsafe to keep a pointer to an object inside a vector if one keeps\n> adding and modifying the vectors content.\n\nVery good point.\n\n> There are also little need to keep the two data structures around when\n> one map can solve the problem. Remove the vector and update all loops\n> to iterate over the map instead of the vector.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/options.cpp | 18 +++++++++++-------\n> src/cam/options.h | 3 +--\n> 2 files changed, 12 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> index c9ca017b4cf3fa3d..d3bff1cd897a5cfb 100644\n> --- a/src/cam/options.cpp\n> +++ b/src/cam/options.cpp\n> @@ -30,8 +30,8 @@ bool OptionsParser::addOption(int opt, const char *help, const char *name,\n> \tif (optionsMap_.find(opt) != optionsMap_.end())\n> \t\treturn false;\n> \n> -\toptions_.push_back(Option({ opt, name, argument, argumentName, help }));\n> -\toptionsMap_[opt] = &options_.back();\n> +\toptionsMap_[opt] = Option({ opt, name, argument, argumentName, help });\n> +\n> \treturn true;\n> }\n> \n> @@ -43,14 +43,16 @@ OptionsParser::Options OptionsParser::parse(int argc, char **argv)\n> \t * Allocate short and long options arrays large enough to contain all\n> \t * options.\n> \t */\n> -\tchar shortOptions[options_.size() * 3 + 2] = {};\n> -\tstruct option longOptions[options_.size() + 1] = {};\n> +\tchar shortOptions[optionsMap_.size() * 3 + 2] = {};\n> +\tstruct option longOptions[optionsMap_.size() + 1] = {};\n> \tunsigned int ids = 0;\n> \tunsigned int idl = 0;\n> \n> \tshortOptions[ids++] = ':';\n> \n> -\tfor (const Option &option : options_) {\n> +\tfor (auto const &iter : optionsMap_) {\n> +\t\tconst Option &option = iter.second;\n> +\n> \t\tif (option.hasShortOption()) {\n> \t\t\tshortOptions[ids++] = option.opt;\n> \t\t\tif (option.argument != ArgumentNone)\n> @@ -112,7 +114,8 @@ void OptionsParser::usage()\n> \n> \tunsigned int indent = 0;\n> \n> -\tfor (const Option &option : options_) {\n> +\tfor (auto const &iter : optionsMap_) {\n> +\t\tconst Option &option = iter.second;\n\nThis will result in options being sorted base on their value. Do we\nreally want this ?\n\nAn alternative would be to replace the std::vector options_ with an\nstd::list that will not invalidate iterators and references when\ninserting an element at the end.\n\n> \t\tunsigned int length = 14;\n> \t\tif (option.hasLongOption())\n> \t\t\tlength += 2 + strlen(option.name);\n> @@ -127,7 +130,8 @@ void OptionsParser::usage()\n> \n> \tindent = (indent + 7) / 8 * 8;\n> \n> -\tfor (const Option &option : options_) {\n> +\tfor (auto const &iter : optionsMap_) {\n> +\t\tconst Option &option = iter.second;\n> \t\tstd::string argument;\n> \t\tif (option.hasShortOption())\n> \t\t\targument = std::string(\" -\")\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> index dfb7fcc9f6fa3324..cb7286a0a8005579 100644\n> --- a/src/cam/options.h\n> +++ b/src/cam/options.h\n> @@ -57,8 +57,7 @@ private:\n> \t\tbool hasLongOption() const { return name != nullptr; }\n> \t};\n> \n> -\tstd::vector