From bd8ff104f5c56e7040041a456a349dac48bf60d9 Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Sun, 19 Jul 2015 09:44:06 +0300 Subject: [PATCH] Add basic grouping to Options - Add simple Options::addGroup() to create groups within an option, allowing out-of-order creation of options. In the future, this can also be used for improved help output. - Replace multiple Options objects in trajectory analysis runner with a single Options and groups within that container. This is a significant step to make also this code use ICommandLineOptionsModule instead of a custom implementation. - Remove complicated code with some dubious corner-case behavior from the command-line parsing code, now that all command-line options are in a single Options object. addSubSection() in Options is now unused, and its general design does not match that of the other Options code. Will refactor and/or remove it separately. Change-Id: Ifaa28a210b46edc0a2b25e7e5c335f0708f532f8 --- src/gromacs/commandline/cmdlineparser.cpp | 3 +- .../commandline/tests/cmdlinehelpwriter.cpp | 19 ++++ ...LineHelpWriterTest_HandlesOptionGroups.xml | 23 ++++ src/gromacs/options/abstractoption.h | 14 ++- src/gromacs/options/options-impl.h | 65 +++++++++-- src/gromacs/options/options.cpp | 105 +++++++++++------- src/gromacs/options/options.h | 24 +++- src/gromacs/options/optionsassigner.cpp | 85 ++------------ src/gromacs/options/optionsassigner.h | 18 +-- src/gromacs/options/optionsvisitor.cpp | 63 +++++++---- src/gromacs/options/tests/optionsassigner.cpp | 78 ++++++------- .../trajectoryanalysis/cmdlinerunner.cpp | 24 ++-- 12 files changed, 283 insertions(+), 238 deletions(-) create mode 100644 src/gromacs/commandline/tests/refdata/CommandLineHelpWriterTest_HandlesOptionGroups.xml diff --git a/src/gromacs/commandline/cmdlineparser.cpp b/src/gromacs/commandline/cmdlineparser.cpp index 61f69d69f7..db654a1f0b 100644 --- a/src/gromacs/commandline/cmdlineparser.cpp +++ b/src/gromacs/commandline/cmdlineparser.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -90,7 +90,6 @@ CommandLineParser::Impl::Impl(Options *options) : assigner_(options), bSkipUnknown_(false) { assigner_.setAcceptBooleanNoPrefix(true); - assigner_.setNoStrictSectioning(true); } const char *CommandLineParser::Impl::toOptionName(const char *arg) const diff --git a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp index 886b0b0d18..49b674c699 100644 --- a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp +++ b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp @@ -246,6 +246,25 @@ TEST_F(CommandLineHelpWriterTest, HandlesSelectionOptions) } #endif +/* + * Tests help output with option groups. + */ +TEST_F(CommandLineHelpWriterTest, HandlesOptionGroups) +{ + using gmx::IntegerOption; + + gmx::Options options(NULL, NULL); + gmx::IOptionsContainer &group1 = options.addGroup(); + gmx::IOptionsContainer &group2 = options.addGroup(); + group2.addOption(IntegerOption("sub2").description("Option in group 2")); + group1.addOption(IntegerOption("sub11").description("Option in group 1")); + options.addOption(IntegerOption("main").description("Option in root group")); + group1.addOption(IntegerOption("sub12").description("Option in group 1")); + + gmx::CommandLineHelpWriter writer(options); + checkHelp(&writer); +} + /* * Tests help output using a help text. */ diff --git a/src/gromacs/commandline/tests/refdata/CommandLineHelpWriterTest_HandlesOptionGroups.xml b/src/gromacs/commandline/tests/refdata/CommandLineHelpWriterTest_HandlesOptionGroups.xml new file mode 100644 index 0000000000..9e1a59586b --- /dev/null +++ b/src/gromacs/commandline/tests/refdata/CommandLineHelpWriterTest_HandlesOptionGroups.xml @@ -0,0 +1,23 @@ + + + + ] [-sub11 ] [-sub12 ] [-sub2 ] + +OPTIONS + +Other options: + + -main + Option in root group + -sub11 + Option in group 1 + -sub12 + Option in group 1 + -sub2 + Option in group 2 + +]]> + diff --git a/src/gromacs/options/abstractoption.h b/src/gromacs/options/abstractoption.h index aa5f2fb1e5..563c3621f6 100644 --- a/src/gromacs/options/abstractoption.h +++ b/src/gromacs/options/abstractoption.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -66,7 +66,11 @@ namespace gmx class AbstractOptionStorage; template class OptionStorageTemplate; class OptionManagerContainer; -class Options; + +namespace internal +{ +class OptionsImpl; +} /*! \brief * Abstract base class for specifying option properties. @@ -190,10 +194,8 @@ class AbstractOption * without otherwise unnecessary accessors. */ friend class AbstractOptionStorage; - /*! \brief - * Needed to be able to call createStorage(). - */ - friend class Options; + //! Needed to be able to call createStorage(). + friend class internal::OptionsImpl; }; /*! \brief diff --git a/src/gromacs/options/options-impl.h b/src/gromacs/options/options-impl.h index a5294c0420..aaf61fb4d8 100644 --- a/src/gromacs/options/options-impl.h +++ b/src/gromacs/options/options-impl.h @@ -42,6 +42,8 @@ #ifndef GMX_OPTIONS_OPTIONS_IMPL_H #define GMX_OPTIONS_OPTIONS_IMPL_H +#include +#include #include #include @@ -55,6 +57,9 @@ namespace gmx class AbstractOptionStorage; +namespace internal +{ + /*! \internal * \brief * Private implementation class for Options. @@ -64,20 +69,53 @@ class AbstractOptionStorage; * * \ingroup module_options */ -class Options::Impl +class OptionsImpl { public: + /*! \internal \brief + * Describes a group of options (see Options::addGroup()). + * + * \ingroup module_options + */ + class Group : public IOptionsContainer + { + public: + //! Convenience typedef for list of options. + typedef std::vector OptionList; + //! Convenience typedef for list of subgroups. + typedef std::list SubgroupList; + + //! Creates a group within the given Options. + explicit Group(OptionsImpl *parent) : parent_(parent) {} + + //! Adds an option subgroup. + IOptionsContainer &addGroup(); + // From IOptionsContainer + virtual OptionInfo *addOption(const AbstractOption &settings); + + //! Containing options object. + OptionsImpl *parent_; + /*! \brief + * List of options, in insertion order. + * + * Pointers in this container point to the objects managed by + * Impl::optionsMap_. + */ + OptionList options_; + //! List of groups, in insertion order. + SubgroupList subgroups_; + }; + //! Smart pointer for managing an AbstractOptionStorage object. typedef gmx_unique_ptr::type AbstractOptionStoragePointer; //! Convenience type for list of sections. typedef std::vector SubSectionList; - //! Convenience type for list of options. - typedef std::vector OptionList; + //! Convenience typedef for a map that contains all the options. + typedef std::map OptionMap; //! Sets the name and title. - Impl(const char *name, const char *title); - ~Impl(); + OptionsImpl(const char *name, const char *title); /*! \brief * Finds a subsection by name. @@ -114,6 +152,14 @@ class Options::Impl * This is non-empty only for the top-level Options object. */ OptionManagerContainer managers_; + /*! \brief + * Group that contains all options (and subgroups). + * + * This is used to store the insertion order of options. + */ + Group rootGroup_; + //! Map from option names to options; owns the option storage objects. + OptionMap optionMap_; /*! \brief * List of subsections, in insertion order. * @@ -121,17 +167,12 @@ class Options::Impl * management is performed elsewhere. */ SubSectionList subSections_; - /*! \brief - * List of options, in insertion order. - * - * All objects in this container are owned by this object, and are - * freed in the destructor. - */ - OptionList options_; //! Options object that contains this object as a subsection, or NULL. Options *parent_; }; +} // namespace internal + } // namespace gmx #endif diff --git a/src/gromacs/options/options.cpp b/src/gromacs/options/options.cpp index 9b422c9640..97dabb99d6 100644 --- a/src/gromacs/options/options.cpp +++ b/src/gromacs/options/options.cpp @@ -43,6 +43,8 @@ #include "options.h" +#include + #include "gromacs/options/abstractoption.h" #include "gromacs/options/abstractoptionstorage.h" #include "gromacs/utility/arrayref.h" @@ -72,20 +74,19 @@ IOptionsContainer::~IOptionsContainer() } /******************************************************************** - * Options::Impl + * OptionsImpl */ -Options::Impl::Impl(const char *name, const char * /*title*/) - : name_(name != NULL ? name : ""), - parent_(NULL) +namespace internal { -} -Options::Impl::~Impl() +OptionsImpl::OptionsImpl(const char *name, const char * /*title*/) + : name_(name != NULL ? name : ""), rootGroup_(this), + parent_(NULL) { } -Options *Options::Impl::findSubSection(const char *name) const +Options *OptionsImpl::findSubSection(const char *name) const { SubSectionList::const_iterator i; for (i = subSections_.begin(); i != subSections_.end(); ++i) @@ -98,25 +99,22 @@ Options *Options::Impl::findSubSection(const char *name) const return NULL; } -AbstractOptionStorage *Options::Impl::findOption(const char *name) const +AbstractOptionStorage *OptionsImpl::findOption(const char *name) const { - OptionList::const_iterator i; - for (i = options_.begin(); i != options_.end(); ++i) + OptionMap::const_iterator i = optionMap_.find(name); + if (i == optionMap_.end()) { - if ((*i)->name() == name) - { - return i->get(); - } + return NULL; } - return NULL; + return i->second.get(); } -void Options::Impl::startSource() +void OptionsImpl::startSource() { - OptionList::const_iterator i; - for (i = options_.begin(); i != options_.end(); ++i) + OptionMap::const_iterator i; + for (i = optionMap_.begin(); i != optionMap_.end(); ++i) { - AbstractOptionStorage &option = **i; + AbstractOptionStorage &option = *i->second; option.startSource(); } SubSectionList::const_iterator j; @@ -127,12 +125,47 @@ void Options::Impl::startSource() } } +/******************************************************************** + * OptionsImpl::Group + */ + +IOptionsContainer &OptionsImpl::Group::addGroup() +{ + subgroups_.push_back(Group(parent_)); + return subgroups_.back(); +} + +OptionInfo *OptionsImpl::Group::addOption(const AbstractOption &settings) +{ + OptionsImpl *root = parent_; + while (root->parent_ != NULL) + { + root = root->parent_->impl_.get(); + } + AbstractOptionStoragePointer option(settings.createStorage(root->managers_)); + options_.reserve(options_.size() + 1); + std::pair insertionResult = + parent_->optionMap_.insert(std::make_pair(option->name(), + move(option))); + if (!insertionResult.second) + { + GMX_THROW(APIError("Duplicate option: " + option->name())); + } + AbstractOptionStorage &insertedOption = *insertionResult.first->second; + options_.push_back(&insertedOption); + return &insertedOption.optionInfo(); +} + +} // namespace internal + +using internal::OptionsImpl; + /******************************************************************** * Options */ Options::Options(const char *name, const char *title) - : impl_(new Impl(name, title)) + : impl_(new OptionsImpl(name, title)) { } @@ -151,7 +184,7 @@ void Options::addManager(IOptionManager *manager) GMX_RELEASE_ASSERT(impl_->parent_ == NULL, "Can only add a manager in a top-level Options object"); // This ensures that all options see the same set of managers. - GMX_RELEASE_ASSERT(impl_->options_.empty(), + GMX_RELEASE_ASSERT(impl_->optionMap_.empty(), "Can only add a manager before options"); // This check could be relaxed if we instead checked that the subsections // do not have options. @@ -164,7 +197,7 @@ void Options::addSubSection(Options *section) { // This is required, because managers are used from the root Options // object, so they are only seen after the subsection has been added. - GMX_RELEASE_ASSERT(section->impl_->options_.empty(), + GMX_RELEASE_ASSERT(section->impl_->optionMap_.empty(), "Can only add a subsection before it has any options"); GMX_RELEASE_ASSERT(section->impl_->managers_.empty(), "Can only have managers in a top-level Options object"); @@ -178,20 +211,14 @@ void Options::addSubSection(Options *section) section->impl_->parent_ = this; } +IOptionsContainer &Options::addGroup() +{ + return impl_->rootGroup_.addGroup(); +} + OptionInfo *Options::addOption(const AbstractOption &settings) { - Options::Impl *root = impl_.get(); - while (root->parent_ != NULL) - { - root = root->parent_->impl_.get(); - } - Impl::AbstractOptionStoragePointer option(settings.createStorage(root->managers_)); - if (impl_->findOption(option->name().c_str()) != NULL) - { - GMX_THROW(APIError("Duplicate option: " + option->name())); - } - impl_->options_.push_back(move(option)); - return &impl_->options_.back()->optionInfo(); + return impl_->rootGroup_.addOption(settings); } bool Options::isSet(const char *name) const @@ -203,11 +230,11 @@ bool Options::isSet(const char *name) const void Options::finish() { // TODO: Consider how to customize these error messages based on context. - ExceptionInitializer errors("Invalid input values"); - Impl::OptionList::const_iterator i; - for (i = impl_->options_.begin(); i != impl_->options_.end(); ++i) + ExceptionInitializer errors("Invalid input values"); + OptionsImpl::OptionMap::const_iterator i; + for (i = impl_->optionMap_.begin(); i != impl_->optionMap_.end(); ++i) { - AbstractOptionStorage &option = **i; + AbstractOptionStorage &option = *i->second; try { option.finish(); @@ -218,7 +245,7 @@ void Options::finish() errors.addCurrentExceptionAsNested(); } } - Impl::SubSectionList::const_iterator j; + OptionsImpl::SubSectionList::const_iterator j; for (j = impl_->subSections_.begin(); j != impl_->subSections_.end(); ++j) { Options §ion = **j; diff --git a/src/gromacs/options/options.h b/src/gromacs/options/options.h index b312218a56..67fcf25737 100644 --- a/src/gromacs/options/options.h +++ b/src/gromacs/options/options.h @@ -58,6 +58,11 @@ class AbstractOption; class OptionsAssigner; class OptionsIterator; +namespace internal +{ +class OptionsImpl; +} + /*! \brief * Base class for option managers. * @@ -153,6 +158,19 @@ class Options : public IOptionsContainer */ void addSubSection(Options *section); + /*! \brief + * Creates a subgroup of options within the current options. + * + * To add options to the group, use the returned interface. + * + * Currently, this is only used to influence the order of options: + * all options in a group appear before options in a group added after + * it, no matter in which order the options are added to the groups. + * In the future, the groups could also be used to influence the help + * output. + */ + IOptionsContainer &addGroup(); + // From IOptionsContainer virtual OptionInfo *addOption(const AbstractOption &settings); using IOptionsContainer::addOption; @@ -176,10 +194,10 @@ class Options : public IOptionsContainer void finish(); private: - class Impl; - - PrivateImplPointer impl_; + PrivateImplPointer impl_; + //! Needed for the implementation to access subsections. + friend class internal::OptionsImpl; //! Needed to be able to extend the interface of this object. friend class OptionsAssigner; //! Needed to be able to extend the interface of this object. diff --git a/src/gromacs/options/optionsassigner.cpp b/src/gromacs/options/optionsassigner.cpp index df3f1320d8..2842655c54 100644 --- a/src/gromacs/options/optionsassigner.cpp +++ b/src/gromacs/options/optionsassigner.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -90,8 +90,6 @@ class OptionsAssigner::Impl Options &options_; //! Recognize boolean option "name" also as "noname". bool bAcceptBooleanNoPrefix_; - //! Look for options in all sections, not just the current one. - bool bNoStrictSectioning_; /*! \brief * List of (sub)sections being assigned to. * @@ -113,8 +111,7 @@ class OptionsAssigner::Impl OptionsAssigner::Impl::Impl(Options *options) : options_(*options), bAcceptBooleanNoPrefix_(false), - bNoStrictSectioning_(false), currentOption_(NULL), - currentValueCount_(0), reverseBoolean_(false) + currentOption_(NULL), currentValueCount_(0), reverseBoolean_(false) { sectionStack_.push_back(&options_); } @@ -124,71 +121,23 @@ OptionsAssigner::Impl::findOption(const char *name) { GMX_RELEASE_ASSERT(currentOption_ == NULL, "Cannot search for another option while processing one"); - AbstractOptionStorage *option = NULL; - Options *section = NULL; - Options *root = ¤tSection(); - Options *oldRoot = NULL; - int upcount = 0; - std::deque searchList; - searchList.push_back(root); - while (option == NULL && !searchList.empty()) + const Options §ion = currentSection(); + AbstractOptionStorage *option = section.impl_->findOption(name); + if (option == NULL && bAcceptBooleanNoPrefix_) { - section = searchList.front(); - option = section->impl_->findOption(name); - if (option == NULL && bAcceptBooleanNoPrefix_) + if (name[0] == 'n' && name[1] == 'o') { - if (name[0] == 'n' && name[1] == 'o') + option = section.impl_->findOption(name + 2); + if (option != NULL && option->isBoolean()) { - option = section->impl_->findOption(name + 2); - if (option != NULL && option->isBoolean()) - { - reverseBoolean_ = true; - } - else - { - option = NULL; - } + reverseBoolean_ = true; } - } - searchList.pop_front(); - if (bNoStrictSectioning_) - { - Options::Impl::SubSectionList::const_iterator i; - for (i = section->impl_->subSections_.begin(); - i != section->impl_->subSections_.end(); ++i) + else { - if (*i != oldRoot) - { - searchList.push_back(*i); - } - } - if (searchList.empty() && root != &options_) - { - root = root->impl_->parent_; - ++upcount; - searchList.push_back(root); + option = NULL; } } } - if (bNoStrictSectioning_ && option != NULL) - { - while (upcount > 0) - { - sectionStack_.pop_back(); - --upcount; - } - std::vector sections; - while (section != ¤tSection()) - { - sections.push_back(section); - section = section->impl_->parent_; - } - while (!sections.empty()) - { - sectionStack_.push_back(sections.back()); - sections.pop_back(); - } - } return option; } @@ -210,11 +159,6 @@ void OptionsAssigner::setAcceptBooleanNoPrefix(bool bEnabled) impl_->bAcceptBooleanNoPrefix_ = bEnabled; } -void OptionsAssigner::setNoStrictSectioning(bool bEnabled) -{ - impl_->bNoStrictSectioning_ = bEnabled; -} - void OptionsAssigner::start() { impl_->options_.impl_->startSource(); @@ -297,13 +241,6 @@ void OptionsAssigner::finishSubSection() void OptionsAssigner::finish() { GMX_RELEASE_ASSERT(impl_->currentOption_ == NULL, "finishOption() not called"); - if (impl_->bNoStrictSectioning_) - { - while (impl_->inSubSection()) - { - finishSubSection(); - } - } GMX_RELEASE_ASSERT(!impl_->inSubSection(), "finishSubSection() not called"); } diff --git a/src/gromacs/options/optionsassigner.h b/src/gromacs/options/optionsassigner.h index 3d06fd2095..35bb70dce9 100644 --- a/src/gromacs/options/optionsassigner.h +++ b/src/gromacs/options/optionsassigner.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -109,22 +109,6 @@ class OptionsAssigner * Does not throw. */ void setAcceptBooleanNoPrefix(bool bEnabled); - /*! \brief - * Sets the assigner to find options in non-active sections. - * - * By default, options are only looked for in the currently active - * subsection. With this option set, if no matching option is found in - * the current section, a breadth-first search is performed, first on - * all subsections of the current section, and then going up one level - * at a time. The first matching option is used, and the current - * section is changed to the section that contains the matching option. - * - * Can be set or cleared at any time, and will have effect on all - * subsequent calls of startOption(). - * - * Does not throw. - */ - void setNoStrictSectioning(bool bEnabled); /*! \brief * Starts assigning values. diff --git a/src/gromacs/options/optionsvisitor.cpp b/src/gromacs/options/optionsvisitor.cpp index 814ae734fd..c9584dee9e 100644 --- a/src/gromacs/options/optionsvisitor.cpp +++ b/src/gromacs/options/optionsvisitor.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2012,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2012,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -51,6 +51,40 @@ namespace gmx { +using internal::OptionsImpl; + +namespace +{ + +//! Helper function to call visitOptions() and handle correct indirection. +void visitOption(OptionsVisitor *visitor, OptionInfo &optionInfo) +{ + visitor->visitOption(optionInfo); +} +//! Helper function to call visitOptions() and handle correct indirection. +void visitOption(OptionsModifyingVisitor *visitor, OptionInfo &optionInfo) +{ + visitor->visitOption(&optionInfo); +} + +//! Helper function to recursively visit all options in a group. +template +void acceptOptionsGroup(const OptionsImpl::Group &group, VisitorType *visitor) +{ + OptionsImpl::Group::OptionList::const_iterator option; + for (option = group.options_.begin(); option != group.options_.end(); ++option) + { + visitOption(visitor, (*option)->optionInfo()); + } + OptionsImpl::Group::SubgroupList::const_iterator subgroup; + for (subgroup = group.subgroups_.begin(); subgroup != group.subgroups_.end(); ++subgroup) + { + acceptOptionsGroup(*subgroup, visitor); + } +} + +} // namespace + /******************************************************************** * OptionsIterator */ @@ -62,9 +96,9 @@ OptionsIterator::OptionsIterator(const Options &options) void OptionsIterator::acceptSubSections(OptionsVisitor *visitor) const { - const Options::Impl::SubSectionList &subSectionList = + const OptionsImpl::SubSectionList &subSectionList = options_.impl_->subSections_; - Options::Impl::SubSectionList::const_iterator i; + OptionsImpl::SubSectionList::const_iterator i; for (i = subSectionList.begin(); i != subSectionList.end(); ++i) { visitor->visitSubSection(*(*i)); @@ -73,16 +107,7 @@ void OptionsIterator::acceptSubSections(OptionsVisitor *visitor) const void OptionsIterator::acceptOptions(OptionsVisitor *visitor) const { - const Options::Impl::OptionList &optionList = - options_.impl_->options_; - Options::Impl::OptionList::const_iterator i; - for (i = optionList.begin(); i != optionList.end(); ++i) - { - // This is not strictly const-correct, since optionInfo() is - // not const (while the input options is), but this makes things much - // simpler. - visitor->visitOption((*i)->optionInfo()); - } + acceptOptionsGroup(options_.impl_->rootGroup_, visitor); } /******************************************************************** @@ -96,9 +121,9 @@ OptionsModifyingIterator::OptionsModifyingIterator(Options *options) void OptionsModifyingIterator::acceptSubSections(OptionsModifyingVisitor *visitor) const { - const Options::Impl::SubSectionList &subSectionList = + const OptionsImpl::SubSectionList &subSectionList = options_.impl_->subSections_; - Options::Impl::SubSectionList::const_iterator i; + OptionsImpl::SubSectionList::const_iterator i; for (i = subSectionList.begin(); i != subSectionList.end(); ++i) { visitor->visitSubSection(*i); @@ -107,13 +132,7 @@ void OptionsModifyingIterator::acceptSubSections(OptionsModifyingVisitor *visito void OptionsModifyingIterator::acceptOptions(OptionsModifyingVisitor *visitor) const { - const Options::Impl::OptionList &optionList = - options_.impl_->options_; - Options::Impl::OptionList::const_iterator i; - for (i = optionList.begin(); i != optionList.end(); ++i) - { - visitor->visitOption(&(*i)->optionInfo()); - } + acceptOptionsGroup(options_.impl_->rootGroup_, visitor); } } // namespace gmx diff --git a/src/gromacs/options/tests/optionsassigner.cpp b/src/gromacs/options/tests/optionsassigner.cpp index cf9648cc1c..0925a4900a 100644 --- a/src/gromacs/options/tests/optionsassigner.cpp +++ b/src/gromacs/options/tests/optionsassigner.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013,2014, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014,2015, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -177,90 +177,74 @@ TEST(OptionsAssignerTest, HandlesExtraValue) EXPECT_EQ(0, value1); } -TEST(OptionsAssignerTest, HandlesSubSections) +TEST(OptionsAssignerTest, HandlesGroups) { - gmx::Options options(NULL, NULL); - gmx::Options sub1("section1", NULL); - gmx::Options sub2("section2", NULL); - int value = 3; - int value1 = 1; - int value2 = 2; + gmx::Options options(NULL, NULL); + gmx::IOptionsContainer &group1 = options.addGroup(); + gmx::IOptionsContainer &group2 = options.addGroup(); + int value = 3; + int value1 = 1; + int value2 = 2; using gmx::IntegerOption; - ASSERT_NO_THROW(options.addSubSection(&sub1)); - ASSERT_NO_THROW(options.addSubSection(&sub2)); ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&value))); - ASSERT_NO_THROW(sub1.addOption(IntegerOption("p").store(&value1))); - ASSERT_NO_THROW(sub2.addOption(IntegerOption("p").store(&value2))); + ASSERT_NO_THROW(group1.addOption(IntegerOption("q").store(&value1))); + ASSERT_NO_THROW(group2.addOption(IntegerOption("r").store(&value2))); gmx::OptionsAssigner assigner(&options); EXPECT_NO_THROW(assigner.start()); - ASSERT_NO_THROW(assigner.startSubSection("section1")); ASSERT_NO_THROW(assigner.startOption("p")); EXPECT_NO_THROW(assigner.appendValue("5")); EXPECT_NO_THROW(assigner.finishOption()); - EXPECT_NO_THROW(assigner.finishSubSection()); - ASSERT_NO_THROW(assigner.startOption("p")); + ASSERT_NO_THROW(assigner.startOption("q")); EXPECT_NO_THROW(assigner.appendValue("4")); EXPECT_NO_THROW(assigner.finishOption()); - ASSERT_NO_THROW(assigner.startSubSection("section2")); - ASSERT_NO_THROW(assigner.startOption("p")); + ASSERT_NO_THROW(assigner.startOption("r")); EXPECT_NO_THROW(assigner.appendValue("6")); EXPECT_NO_THROW(assigner.finishOption()); - EXPECT_NO_THROW(assigner.finishSubSection()); EXPECT_NO_THROW(assigner.finish()); EXPECT_NO_THROW(options.finish()); - EXPECT_EQ(4, value); - EXPECT_EQ(5, value1); + EXPECT_EQ(5, value); + EXPECT_EQ(4, value1); EXPECT_EQ(6, value2); } -TEST(OptionsAssignerTest, HandlesNoStrictSubSections) +TEST(OptionsAssignerTest, HandlesSubSections) { gmx::Options options(NULL, NULL); gmx::Options sub1("section1", NULL); gmx::Options sub2("section2", NULL); - int pvalue = 3; - int pvalue1 = 1; - int qvalue = 4; - int pvalue2 = 2; - int rvalue = 5; + int value = 3; + int value1 = 1; + int value2 = 2; using gmx::IntegerOption; ASSERT_NO_THROW(options.addSubSection(&sub1)); ASSERT_NO_THROW(options.addSubSection(&sub2)); - ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&pvalue))); - ASSERT_NO_THROW(sub1.addOption(IntegerOption("p").store(&pvalue1))); - ASSERT_NO_THROW(sub1.addOption(IntegerOption("q").store(&qvalue))); - ASSERT_NO_THROW(sub2.addOption(IntegerOption("p").store(&pvalue2))); - ASSERT_NO_THROW(sub2.addOption(IntegerOption("r").store(&rvalue))); + ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&value))); + ASSERT_NO_THROW(sub1.addOption(IntegerOption("p").store(&value1))); + ASSERT_NO_THROW(sub2.addOption(IntegerOption("p").store(&value2))); gmx::OptionsAssigner assigner(&options); - assigner.setNoStrictSectioning(true); EXPECT_NO_THROW(assigner.start()); - ASSERT_NO_THROW(assigner.startOption("q")); - EXPECT_NO_THROW(assigner.appendValue("6")); - EXPECT_NO_THROW(assigner.finishOption()); + ASSERT_NO_THROW(assigner.startSubSection("section1")); ASSERT_NO_THROW(assigner.startOption("p")); - EXPECT_NO_THROW(assigner.appendValue("7")); - EXPECT_NO_THROW(assigner.finishOption()); - ASSERT_NO_THROW(assigner.startOption("r")); - EXPECT_NO_THROW(assigner.appendValue("8")); + EXPECT_NO_THROW(assigner.appendValue("5")); EXPECT_NO_THROW(assigner.finishOption()); + EXPECT_NO_THROW(assigner.finishSubSection()); ASSERT_NO_THROW(assigner.startOption("p")); - EXPECT_NO_THROW(assigner.appendValue("9")); + EXPECT_NO_THROW(assigner.appendValue("4")); EXPECT_NO_THROW(assigner.finishOption()); - EXPECT_NO_THROW(assigner.finishSubSection()); + ASSERT_NO_THROW(assigner.startSubSection("section2")); ASSERT_NO_THROW(assigner.startOption("p")); - EXPECT_NO_THROW(assigner.appendValue("10")); + EXPECT_NO_THROW(assigner.appendValue("6")); EXPECT_NO_THROW(assigner.finishOption()); + EXPECT_NO_THROW(assigner.finishSubSection()); EXPECT_NO_THROW(assigner.finish()); EXPECT_NO_THROW(options.finish()); - EXPECT_EQ(6, qvalue); - EXPECT_EQ(7, pvalue1); - EXPECT_EQ(8, rvalue); - EXPECT_EQ(9, pvalue2); - EXPECT_EQ(10, pvalue); + EXPECT_EQ(4, value); + EXPECT_EQ(5, value1); + EXPECT_EQ(6, value2); } TEST(OptionsAssignerTest, HandlesMultipleSources) diff --git a/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp b/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp index 5ac15b6281..a32a591e35 100644 --- a/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp +++ b/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp @@ -111,19 +111,15 @@ TrajectoryAnalysisCommandLineRunner::Impl::parseOptions( FileNameOptionManager fileoptManager; SelectionOptionManager seloptManager(selections); Options options(NULL, NULL); - Options moduleOptions(module_->name(), module_->description()); - Options commonOptions("common", "Common analysis control"); - Options selectionOptions("selection", "Common selection control"); options.addManager(&fileoptManager); options.addManager(&seloptManager); - options.addSubSection(&commonOptions); - options.addSubSection(&selectionOptions); - options.addSubSection(&moduleOptions); + IOptionsContainer &commonOptions = options.addGroup(); + IOptionsContainer &moduleOptions = options.addGroup(); module_->initOptions(&moduleOptions, settings); common->initOptions(&commonOptions); - selections->initOptions(&selectionOptions); + selections->initOptions(&commonOptions); { CommandLineParser parser(&options); @@ -134,8 +130,8 @@ TrajectoryAnalysisCommandLineRunner::Impl::parseOptions( options.finish(); } - common->optionsFinished(&commonOptions); - module_->optionsFinished(&moduleOptions, settings); + common->optionsFinished(&options); + module_->optionsFinished(&options, settings); common->initIndexGroups(selections, bUseDefaultGroups_); @@ -259,18 +255,14 @@ TrajectoryAnalysisCommandLineRunner::writeHelp(const CommandLineHelpContext &con SelectionOptionManager seloptManager(&selections); Options options(NULL, NULL); - Options moduleOptions(impl_->module_->name(), impl_->module_->description()); - Options commonOptions("common", "Common analysis control"); - Options selectionOptions("selection", "Common selection control"); options.addManager(&seloptManager); - options.addSubSection(&commonOptions); - options.addSubSection(&selectionOptions); - options.addSubSection(&moduleOptions); + IOptionsContainer &commonOptions = options.addGroup(); + IOptionsContainer &moduleOptions = options.addGroup(); impl_->module_->initOptions(&moduleOptions, &settings); common.initOptions(&commonOptions); - selections.initOptions(&selectionOptions); + selections.initOptions(&commonOptions); CommandLineHelpWriter(options) .setHelpText(settings.helpText()) -- 2.22.0