| Message ID | 20190128004109.25860-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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__ */
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; 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_; }; #endif /* __CAM_OPTIONS_H__ */
It's unsafe to keep a pointer to an object inside a vector if one keeps adding and modifying the vectors content. 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@ragnatech.se> --- src/cam/options.cpp | 18 +++++++++++------- src/cam/options.h | 3 +-- 2 files changed, 12 insertions(+), 9 deletions(-)