From 1d6ad5df361f93ed933e33134b2dba9297c56e5c Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Fri, 9 May 2014 07:40:21 +0300 Subject: [PATCH] Improve (Selection|FileName)OptionManager handling The way the managers get passed to the option storage classes is now reversed: the manager is passed already to the constructor. This has several benefits: - Already the constructor can use the manager if necessary. - It is possible to easily assert on all incorrect usage scenarios. - There is much less boilerplate code associated with propagating the manager into the storage classes. There is also less code in general: even with an addition of a new file and its copyright header, as well as more extensive documentation, there are still fewer total lines with the new approach. The minor disadvantage is that also option types that do not use a manager get the container passed as an argument to createStorage(), but they can just ignore the parameter, so the above benefits should clearly outweigh this. There was an outdated comment in options.h about the usage of addSubSection(), which is fixed here. It is now necessary to add subsections before any options are added to them, but this is likely a reasonable constraint for some other future stuff as well. Change-Id: I4a5e69d3fd596dbe265613f81399219f8b8b19c8 --- src/gromacs/commandline/pargs.cpp | 3 +- .../commandline/tests/cmdlinehelpwriter.cpp | 8 +- src/gromacs/options/abstractoption.h | 10 +- src/gromacs/options/basicoptions.cpp | 18 ++- src/gromacs/options/basicoptions.h | 18 ++- src/gromacs/options/filenameoption.cpp | 32 ++--- src/gromacs/options/filenameoption.h | 19 +-- src/gromacs/options/filenameoptionmanager.cpp | 51 ------- src/gromacs/options/filenameoptionmanager.h | 24 +--- src/gromacs/options/filenameoptionstorage.h | 13 +- src/gromacs/options/optionmanagercontainer.h | 124 ++++++++++++++++++ src/gromacs/options/options-impl.h | 9 +- src/gromacs/options/options.cpp | 35 ++++- src/gromacs/options/options.h | 48 ++++++- .../options/tests/abstractoptionstorage.cpp | 3 +- src/gromacs/options/tests/filenameoption.cpp | 4 +- src/gromacs/options/tests/optionsassigner.cpp | 10 +- src/gromacs/selection/selectionfileoption.h | 12 +- .../selection/selectionfileoptionstorage.h | 16 +-- src/gromacs/selection/selectionoption.cpp | 79 ++++------- src/gromacs/selection/selectionoption.h | 23 +--- .../selection/selectionoptionmanager.cpp | 57 -------- .../selection/selectionoptionmanager.h | 23 +--- .../selection/selectionoptionstorage.h | 9 +- .../selection/tests/selectionoption.cpp | 32 +---- .../trajectoryanalysis/cmdlinerunner.cpp | 37 +++--- 26 files changed, 350 insertions(+), 367 deletions(-) create mode 100644 src/gromacs/options/optionmanagercontainer.h diff --git a/src/gromacs/commandline/pargs.cpp b/src/gromacs/commandline/pargs.cpp index 151c3b9e50..c5e2cd3664 100644 --- a/src/gromacs/commandline/pargs.cpp +++ b/src/gromacs/commandline/pargs.cpp @@ -502,6 +502,7 @@ gmx_bool parse_common_args(int *argc, char *argv[], unsigned long Flags, gmx::Options options(NULL, NULL); gmx::FileNameOptionManager fileOptManager; + options.addManager(&fileOptManager); options.setDescription(gmx::ConstArrayRef(desc, ndesc)); options.addOption( gmx::IntegerOption("debug").store(&debug_level).hidden() @@ -572,8 +573,6 @@ gmx_bool parse_common_args(int *argc, char *argv[], unsigned long Flags, adapter.pargsToOptions(&options, &pa[i]); } - setManagerForFileNameOptions(&options, &fileOptManager); - const gmx::CommandLineHelpContext *context = gmx::GlobalCommandLineHelpContext::get(); if (context != NULL) diff --git a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp index b8e5b3adc9..5d0e7debab 100644 --- a/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp +++ b/src/gromacs/commandline/tests/cmdlinehelpwriter.cpp @@ -223,15 +223,15 @@ TEST_F(CommandLineHelpWriterTest, HandlesSelectionOptions) using gmx::SelectionFileOption; using gmx::SelectionOption; - gmx::Options options(NULL, NULL); + gmx::Options options(NULL, NULL); + gmx::SelectionCollection selections; + gmx::SelectionOptionManager manager(&selections); + options.addManager(&manager); options.addOption(SelectionFileOption("sf")); options.addOption(SelectionOption("refsel").required() .description("Reference selection option")); options.addOption(SelectionOption("sel").required().valueCount(2) .description("Selection option")); - gmx::SelectionCollection selections; - gmx::SelectionOptionManager manager(&selections); - setManagerForSelectionOptions(&options, &manager); options.finish(); manager.parseRequestedFromString( "resname SOL;" diff --git a/src/gromacs/options/abstractoption.h b/src/gromacs/options/abstractoption.h index ea365d2773..4c0153b834 100644 --- a/src/gromacs/options/abstractoption.h +++ b/src/gromacs/options/abstractoption.h @@ -67,6 +67,7 @@ namespace gmx class AbstractOptionStorage; template class OptionStorageTemplate; +class OptionManagerContainer; class Options; //! Smart pointer for managing an AbstractOptionStorage object. @@ -108,8 +109,10 @@ class AbstractOption /*! \brief * Creates a default storage object for the option. * - * \returns The created storage object. - * \throws APIError if invalid option settings have been provided. + * \param[in] managers Manager container (unused if the option does + * not use a manager). + * \returns The created storage object. + * \throws APIError if invalid option settings have been provided. * * This method is called by Options::addOption() when initializing an * option from the settings. @@ -120,7 +123,8 @@ class AbstractOption * * Should only be called by Options::addOption(). */ - virtual AbstractOptionStoragePointer createStorage() const = 0; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const = 0; //! Sets the description for the option. void setDescription(const char *descr) { descr_ = descr; } diff --git a/src/gromacs/options/basicoptions.cpp b/src/gromacs/options/basicoptions.cpp index f1af57aa32..205a3c1023 100644 --- a/src/gromacs/options/basicoptions.cpp +++ b/src/gromacs/options/basicoptions.cpp @@ -138,7 +138,8 @@ bool BooleanOptionInfo::defaultValue() const * BooleanOption */ -AbstractOptionStoragePointer BooleanOption::createStorage() const +AbstractOptionStoragePointer +BooleanOption::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new BooleanOptionStorage(*this)); } @@ -195,7 +196,8 @@ IntegerOptionInfo::IntegerOptionInfo(IntegerOptionStorage *option) * IntegerOption */ -AbstractOptionStoragePointer IntegerOption::createStorage() const +AbstractOptionStoragePointer +IntegerOption::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new IntegerOptionStorage(*this)); } @@ -242,7 +244,8 @@ Int64OptionInfo::Int64OptionInfo(Int64OptionStorage *option) * Int64Option */ -AbstractOptionStoragePointer Int64Option::createStorage() const +AbstractOptionStoragePointer +Int64Option::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new Int64OptionStorage(*this)); } @@ -343,7 +346,8 @@ void DoubleOptionInfo::setScaleFactor(double factor) * DoubleOption */ -AbstractOptionStoragePointer DoubleOption::createStorage() const +AbstractOptionStoragePointer +DoubleOption::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new DoubleOptionStorage(*this)); } @@ -446,7 +450,8 @@ void FloatOptionInfo::setScaleFactor(double factor) * FloatOption */ -AbstractOptionStoragePointer FloatOption::createStorage() const +AbstractOptionStoragePointer +FloatOption::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new FloatOptionStorage(*this)); } @@ -631,7 +636,8 @@ const std::vector &StringOptionInfo::allowedValues() const * StringOption */ -AbstractOptionStoragePointer StringOption::createStorage() const +AbstractOptionStoragePointer +StringOption::createStorage(const OptionManagerContainer & /*managers*/) const { return AbstractOptionStoragePointer(new StringOptionStorage(*this)); } diff --git a/src/gromacs/options/basicoptions.h b/src/gromacs/options/basicoptions.h index 35e91cc817..992d19ed00 100644 --- a/src/gromacs/options/basicoptions.h +++ b/src/gromacs/options/basicoptions.h @@ -98,7 +98,8 @@ class BooleanOption : public OptionTemplate private: //! Creates a BooleanOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; }; /*! \brief @@ -140,7 +141,8 @@ class IntegerOption : public OptionTemplate private: //! Creates an IntegerOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; /*! \brief * Needed to initialize IntegerOptionStorage from this class without @@ -169,7 +171,8 @@ class Int64Option : public OptionTemplate private: //! Creates an Int64OptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; /*! \brief * Needed to initialize Int64OptionStorage from this class without @@ -214,7 +217,8 @@ class DoubleOption : public OptionTemplate private: //! Creates a DoubleOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; bool bTime_; @@ -252,7 +256,8 @@ class FloatOption : public OptionTemplate private: //! Creates a FloatOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; bool bTime_; @@ -372,7 +377,8 @@ class StringOption : public OptionTemplate private: //! Creates a StringOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; const char *const *enumValues_; int enumValuesCount_; diff --git a/src/gromacs/options/filenameoption.cpp b/src/gromacs/options/filenameoption.cpp index 9922e16fa9..b56ba9e012 100644 --- a/src/gromacs/options/filenameoption.cpp +++ b/src/gromacs/options/filenameoption.cpp @@ -49,6 +49,7 @@ #include "gromacs/fileio/filenm.h" #include "gromacs/options/filenameoptionmanager.h" +#include "gromacs/options/optionmanagercontainer.h" #include "gromacs/utility/arrayref.h" #include "gromacs/utility/file.h" #include "gromacs/utility/gmxassert.h" @@ -294,8 +295,9 @@ std::string completeFileName(const std::string &value, OptionFileType filetype, * FileNameOptionStorage */ -FileNameOptionStorage::FileNameOptionStorage(const FileNameOption &settings) - : MyBase(settings), info_(this), manager_(NULL), +FileNameOptionStorage::FileNameOptionStorage(const FileNameOption &settings, + FileNameOptionManager *manager) + : MyBase(settings), info_(this), manager_(manager), filetype_(settings.filetype_), legacyType_(settings.legacyType_), bRead_(settings.bRead_), bWrite_(settings.bWrite_), bLibrary_(settings.bLibrary_) @@ -315,16 +317,6 @@ FileNameOptionStorage::FileNameOptionStorage(const FileNameOption &settings) } } -void FileNameOptionStorage::setManager(FileNameOptionManager *manager) -{ - GMX_RELEASE_ASSERT(manager_ == NULL || manager_ == manager, - "Manager cannot be changed once set"); - if (manager_ == NULL) - { - manager_ = manager; - } -} - std::string FileNameOptionStorage::typeString() const { const FileTypeRegistry ®istry = FileTypeRegistry::instance(); @@ -442,21 +434,11 @@ FileNameOptionInfo::FileNameOptionInfo(FileNameOptionStorage *option) { } -FileNameOptionStorage &FileNameOptionInfo::option() -{ - return static_cast(OptionInfo::option()); -} - const FileNameOptionStorage &FileNameOptionInfo::option() const { return static_cast(OptionInfo::option()); } -void FileNameOptionInfo::setManager(FileNameOptionManager *manager) -{ - option().setManager(manager); -} - bool FileNameOptionInfo::isInputFile() const { return option().isInputFile(); @@ -491,9 +473,11 @@ FileNameOptionInfo::ExtensionList FileNameOptionInfo::extensions() const * FileNameOption */ -AbstractOptionStoragePointer FileNameOption::createStorage() const +AbstractOptionStoragePointer +FileNameOption::createStorage(const OptionManagerContainer &managers) const { - return AbstractOptionStoragePointer(new FileNameOptionStorage(*this)); + return AbstractOptionStoragePointer( + new FileNameOptionStorage(*this, managers.get())); } } // namespace gmx diff --git a/src/gromacs/options/filenameoption.h b/src/gromacs/options/filenameoption.h index 45fd6fa1a9..7b5abb146d 100644 --- a/src/gromacs/options/filenameoption.h +++ b/src/gromacs/options/filenameoption.h @@ -165,7 +165,8 @@ class FileNameOption : public OptionTemplate using MyBase::defaultValueIfSet; //! Creates a FileNameOptionStorage object. - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; OptionFileType filetype_; int legacyType_; @@ -197,21 +198,6 @@ class FileNameOptionInfo : public OptionInfo //! Creates an option info object for the given option. explicit FileNameOptionInfo(FileNameOptionStorage *option); - /*! \brief - * Set manager for handling interaction with other options. - * - * \param manager File name option manager to set. - * - * This must be called before the values are added. - * - * Typically it is called through setManagerForFileNameOptions(), - * which recursively sets the manager for all file name options in - * an Options object. - * - * Does not throw. - */ - void setManager(FileNameOptionManager *manager); - //! Whether the option specifies an input file. bool isInputFile() const; //! Whether the option specifies an output file. @@ -231,7 +217,6 @@ class FileNameOptionInfo : public OptionInfo ExtensionList extensions() const; private: - FileNameOptionStorage &option(); const FileNameOptionStorage &option() const; }; diff --git a/src/gromacs/options/filenameoptionmanager.cpp b/src/gromacs/options/filenameoptionmanager.cpp index db7b2fde10..3ba0aeac41 100644 --- a/src/gromacs/options/filenameoptionmanager.cpp +++ b/src/gromacs/options/filenameoptionmanager.cpp @@ -46,7 +46,6 @@ #include "gromacs/options/basicoptions.h" #include "gromacs/options/filenameoption.h" #include "gromacs/options/options.h" -#include "gromacs/options/optionsvisitor.h" namespace gmx { @@ -93,54 +92,4 @@ const std::string &FileNameOptionManager::defaultFileName() const return impl_->defaultFileName_; } -/******************************************************************** - * Global functions - */ - -namespace -{ - -/*! \internal \brief - * Visitor that sets the manager for each file name option. - * - * \ingroup module_options - */ -class FileNameOptionManagerSetter : public OptionsModifyingVisitor -{ - public: - //! Construct a visitor that sets given manager. - explicit FileNameOptionManagerSetter(FileNameOptionManager *manager) - : manager_(manager) - { - } - - void visitSubSection(Options *section) - { - OptionsModifyingIterator iterator(section); - iterator.acceptSubSections(this); - iterator.acceptOptions(this); - } - - void visitOption(OptionInfo *option) - { - FileNameOptionInfo *fileOption - = option->toType(); - if (fileOption != NULL) - { - fileOption->setManager(manager_); - } - } - - private: - FileNameOptionManager *manager_; -}; - -} // namespace - -void setManagerForFileNameOptions(Options *options, - FileNameOptionManager *manager) -{ - FileNameOptionManagerSetter(manager).visitSubSection(options); -} - } // namespace gmx diff --git a/src/gromacs/options/filenameoptionmanager.h b/src/gromacs/options/filenameoptionmanager.h index 24853ce7b7..caa9644363 100644 --- a/src/gromacs/options/filenameoptionmanager.h +++ b/src/gromacs/options/filenameoptionmanager.h @@ -45,6 +45,8 @@ #include +#include "options.h" + #include "../utility/common.h" namespace gmx @@ -64,20 +66,20 @@ class Options; * most of the non-trivial file name completion logic in this class, so that * the customizations would be centralized here. * - * Creating a FileNameOptionManager for an Options object is optional, even if + * Adding a FileNameOptionManager for an Options object is optional, even if * the Options contains FileNameOption options. Features from the manager are * not available if the manager is not created, but otherwise the options work. * - * \see setManagerForFileNameOptions() + * \see Options::addManager() * * \inpublicapi * \ingroup module_selection */ -class FileNameOptionManager +class FileNameOptionManager : public OptionManagerInterface { public: FileNameOptionManager(); - ~FileNameOptionManager(); + virtual ~FileNameOptionManager(); /*! \brief * Adds an option for setting the default global file name. @@ -102,20 +104,6 @@ class FileNameOptionManager PrivateImplPointer impl_; }; -/*! \brief - * Set manager for all file name options. - * - * Recursively sets the manager to \p manager for all file name options in - * \p options. - * Must be called before value assignment starts for \p options. - * - * Does not throw. - * - * \inpublicapi - */ -void setManagerForFileNameOptions(Options *options, - FileNameOptionManager *manager); - } // namespace gmx #endif diff --git a/src/gromacs/options/filenameoptionstorage.h b/src/gromacs/options/filenameoptionstorage.h index e904707758..0586bbae88 100644 --- a/src/gromacs/options/filenameoptionstorage.h +++ b/src/gromacs/options/filenameoptionstorage.h @@ -60,17 +60,20 @@ class FileNameOptionManager; class FileNameOptionStorage : public OptionStorageTemplate { public: - //! \copydoc StringOptionStorage::StringOptionStorage() - explicit FileNameOptionStorage(const FileNameOption &settings); + /*! \brief + * Initializes the storage from option settings. + * + * \param[in] settings Storage settings. + * \param manager Manager for this object (can be NULL). + */ + FileNameOptionStorage(const FileNameOption &settings, + FileNameOptionManager *manager); virtual OptionInfo &optionInfo() { return info_; } virtual std::string typeString() const; virtual std::string formatExtraDescription() const; virtual std::string formatSingleValue(const std::string &value) const; - //! \copydoc FileNameOptionInfo::setManager() - void setManager(FileNameOptionManager *manager); - //! \copydoc FileNameOptionInfo::isInputFile() bool isInputFile() const { return bRead_ && !bWrite_; } //! \copydoc FileNameOptionInfo::isOutputFile() diff --git a/src/gromacs/options/optionmanagercontainer.h b/src/gromacs/options/optionmanagercontainer.h new file mode 100644 index 0000000000..b282cd6390 --- /dev/null +++ b/src/gromacs/options/optionmanagercontainer.h @@ -0,0 +1,124 @@ +/* + * This file is part of the GROMACS molecular simulation package. + * + * Copyright (c) 2014, 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. + * + * GROMACS is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public License + * as published by the Free Software Foundation; either version 2.1 + * of the License, or (at your option) any later version. + * + * GROMACS is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with GROMACS; if not, see + * http://www.gnu.org/licenses, or write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + * If you want to redistribute modifications to GROMACS, please + * consider that scientific software is very special. Version + * control is crucial - bugs must be traceable. We will be happy to + * consider code for inclusion in the official distribution, but + * derived work must not be called official GROMACS. Details are found + * in the README & COPYING files - if they are missing, get the + * official version at http://www.gromacs.org. + * + * To help us fund GROMACS development, we humbly ask that you cite + * the research papers on the package. Check out http://www.gromacs.org. + */ +/*! \libinternal \file + * \brief + * Declares gmx::OptionManagerContainer. + * + * \author Teemu Murtola + * \inlibraryapi + * \ingroup module_options + */ +#ifndef GMX_OPTIONS_OPTIONMANAGERCONTAINER_H +#define GMX_OPTIONS_OPTIONMANAGERCONTAINER_H + +#include + +#include "gromacs/utility/common.h" +#include "gromacs/utility/gmxassert.h" + +namespace gmx +{ + +class OptionManagerInterface; + +/*! \libinternal + * \brief + * Container to keep managers added with Options::addManager() and pass them + * to options. + * + * Consistency of the managers (e.g., that there is at most one manager of a + * certain type) is only checked when the managers are accessed. + * + * \inlibraryapi + * \ingroup module_options + */ +class OptionManagerContainer +{ + public: + OptionManagerContainer() + { + } + + //! Returns `true` if there are no managers. + bool empty() const { return list_.empty(); } + + //! Adds a manager to the container. + void add(OptionManagerInterface *manager) + { + list_.push_back(manager); + } + /*! \brief + * Retrieves a manager of a certain type. + * + * \tparam ManagerType Type of manager to retrieve + * (should derive from OptionManagerInterface). + * \returns The manager, or `NULL` if there is none. + * + * This method is used in AbstractOption::createStorage() to retrieve + * a manager of a certain type for options that use a manager. + * + * The return value is `NULL` if there is no manager of the given type. + * The caller needs to handle this (either by asserting, or by handling + * the manager as optional). + */ + template + ManagerType *get() const + { + ManagerType *result = NULL; + for (ListType::const_iterator i = list_.begin(); i != list_.end(); ++i) + { + ManagerType *curr = dynamic_cast(*i); + if (curr != NULL) + { + GMX_RELEASE_ASSERT(result == NULL, + "More than one applicable option manager is set"); + result = curr; + } + } + return result; + } + + private: + //! Shorthand for the internal container type. + typedef std::vector ListType; + + ListType list_; + + GMX_DISALLOW_COPY_AND_ASSIGN(OptionManagerContainer); +}; + +} // namespace gmx + +#endif diff --git a/src/gromacs/options/options-impl.h b/src/gromacs/options/options-impl.h index 6b06f1fcd7..c126f07e85 100644 --- a/src/gromacs/options/options-impl.h +++ b/src/gromacs/options/options-impl.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2010,2011,2012,2013, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014, 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. @@ -46,6 +46,7 @@ #include #include "abstractoption.h" +#include "optionmanagercontainer.h" #include "options.h" namespace gmx @@ -107,6 +108,12 @@ class Options::Impl std::string title_; //! Full description for the Options object. std::string description_; + /*! \brief + * Option managers set for this collection. + * + * This is non-empty only for the top-level Options object. + */ + OptionManagerContainer managers_; /*! \brief * List of subsections, in insertion order. * diff --git a/src/gromacs/options/options.cpp b/src/gromacs/options/options.cpp index 8b3c22e264..b64f285268 100644 --- a/src/gromacs/options/options.cpp +++ b/src/gromacs/options/options.cpp @@ -54,6 +54,14 @@ namespace gmx { +/******************************************************************** + * OptionManagerInterface + */ + +OptionManagerInterface::~OptionManagerInterface() +{ +} + /******************************************************************** * Options::Impl */ @@ -148,8 +156,28 @@ void Options::setDescription(const ConstArrayRef &descArray) impl_->description_ = concatenateStrings(descArray.data(), descArray.size()); } +void Options::addManager(OptionManagerInterface *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(), + "Can only add a manager before options"); + // This check could be relaxed if we instead checked that the subsections + // do not have options. + GMX_RELEASE_ASSERT(impl_->subSections_.empty(), + "Can only add a manager before subsections"); + impl_->managers_.add(manager); +} + 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(), + "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"); // Make sure that section is not already inserted somewhere. GMX_RELEASE_ASSERT(section->impl_->parent_ == NULL, "Cannot add as subsection twice"); @@ -162,7 +190,12 @@ void Options::addSubSection(Options *section) OptionInfo *Options::addOption(const AbstractOption &settings) { - AbstractOptionStoragePointer option(settings.createStorage()); + Options::Impl *root = impl_.get(); + while (root->parent_ != NULL) + { + root = root->parent_->impl_.get(); + } + AbstractOptionStoragePointer option(settings.createStorage(root->managers_)); if (impl_->findOption(option->name().c_str()) != NULL) { GMX_THROW(APIError("Duplicate option: " + option->name())); diff --git a/src/gromacs/options/options.h b/src/gromacs/options/options.h index cd5f1dc7b0..7623086796 100644 --- a/src/gromacs/options/options.h +++ b/src/gromacs/options/options.h @@ -62,6 +62,27 @@ class AbstractOption; class OptionsAssigner; class OptionsIterator; +/*! \brief + * Base class for option managers. + * + * This class is used as a marker for all classes that are used with + * Options::addManager(). It doesn't provide any methods, but only supports + * transporting these classes through the Options collection into the + * individual option implementation classes. + * + * The virtual destructor is present to make this class polymorphic, such that + * `dynamic_cast` can be used when retrieving a manager of a certain type for + * the individual options. + * + * \inlibraryapi + * \ingroup module_options + */ +class OptionManagerInterface +{ + protected: + virtual ~OptionManagerInterface(); +}; + /*! \brief * Collection of options. * @@ -152,6 +173,28 @@ class Options */ void setDescription(const ConstArrayRef &descArray); + /*! \brief + * Adds an option manager. + * + * \param manager Manager to add. + * \throws std::bad_alloc if out of memory. + * + * Option managers are used by some types of options that require + * interaction between different option instances (e.g., selection + * options), or need to support globally set properties (e.g., a global + * default file prefix). Option objects can retrieve the pointer to + * their manager when they are created, and the caller can alter the + * behavior of the options through the manager. + * See the individual managers for details. + * + * Caller is responsible for memory management of \p manager. + * The Options object (and its contained options) only stores a + * reference to the object. + * + * This method cannot be called after adding options or subsections. + */ + void addManager(OptionManagerInterface *manager); + /*! \brief * Adds an option collection as a subsection of this collection. * @@ -161,9 +204,8 @@ class Options * subsection. If an attempt is made to add two different subsections * with the same name, this function asserts. * - * For certain functionality to work properly, no options should - * be added to the subsection after it has been added to another - * collection. + * \p section should not have any options added at the point this + * method is called. * * Only a pointer to the provided object is stored. The caller is * responsible that the object exists for the lifetime of the diff --git a/src/gromacs/options/tests/abstractoptionstorage.cpp b/src/gromacs/options/tests/abstractoptionstorage.cpp index 716e0b1f56..6df36ee6eb 100644 --- a/src/gromacs/options/tests/abstractoptionstorage.cpp +++ b/src/gromacs/options/tests/abstractoptionstorage.cpp @@ -151,7 +151,8 @@ class MockOption : public gmx::OptionTemplate } private: - virtual gmx::AbstractOptionStoragePointer createStorage() const + virtual gmx::AbstractOptionStoragePointer createStorage( + const gmx::OptionManagerContainer & /*managers*/) const { return gmx::AbstractOptionStoragePointer(new MockOptionStorage(*this)); } diff --git a/src/gromacs/options/tests/filenameoption.cpp b/src/gromacs/options/tests/filenameoption.cpp index 949928c182..be0acd969b 100644 --- a/src/gromacs/options/tests/filenameoption.cpp +++ b/src/gromacs/options/tests/filenameoption.cpp @@ -237,12 +237,12 @@ TEST(FileNameOptionTest, AddsMissingExtensionForRequiredFromDefaultNameOptionBas gmx::FileNameOptionManager manager; gmx::Options options(NULL, NULL); std::string value; + ASSERT_NO_THROW_GMX(options.addManager(&manager)); ASSERT_NO_THROW_GMX(options.addOption( FileNameOption("f").store(&value).required() .filetype(gmx::eftTrajectory).inputFile() .defaultBasename("foo"))); ASSERT_NO_THROW_GMX(manager.addDefaultFileNameOption(&options, "deffnm")); - ASSERT_NO_THROW_GMX(setManagerForFileNameOptions(&options, &manager)); EXPECT_EQ("foo.xtc", value); gmx::OptionsAssigner assigner(&options); @@ -266,12 +266,12 @@ TEST(FileNameOptionTest, AddsMissingExtensionForOptionalFromDefaultNameOptionBas gmx::FileNameOptionManager manager; gmx::Options options(NULL, NULL); std::string value; + ASSERT_NO_THROW_GMX(options.addManager(&manager)); ASSERT_NO_THROW_GMX(options.addOption( FileNameOption("f").store(&value) .filetype(gmx::eftTrajectory).inputFile() .defaultBasename("foo"))); ASSERT_NO_THROW_GMX(manager.addDefaultFileNameOption(&options, "deffnm")); - ASSERT_NO_THROW_GMX(setManagerForFileNameOptions(&options, &manager)); gmx::OptionsAssigner assigner(&options); EXPECT_NO_THROW_GMX(assigner.start()); diff --git a/src/gromacs/options/tests/optionsassigner.cpp b/src/gromacs/options/tests/optionsassigner.cpp index e12d787ba2..8fb44f0bd4 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, by the GROMACS development team, led by + * Copyright (c) 2010,2011,2012,2013,2014, 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. @@ -183,11 +183,11 @@ TEST(OptionsAssignerTest, HandlesSubSections) 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(options.addSubSection(&sub1)); - ASSERT_NO_THROW(options.addSubSection(&sub2)); gmx::OptionsAssigner assigner(&options); EXPECT_NO_THROW(assigner.start()); @@ -223,13 +223,13 @@ TEST(OptionsAssignerTest, HandlesNoStrictSubSections) int pvalue2 = 2; int rvalue = 5; 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.addSubSection(&sub1)); - ASSERT_NO_THROW(options.addSubSection(&sub2)); gmx::OptionsAssigner assigner(&options); assigner.setNoStrictSectioning(true); diff --git a/src/gromacs/selection/selectionfileoption.h b/src/gromacs/selection/selectionfileoption.h index c0a70dad00..773bff96b3 100644 --- a/src/gromacs/selection/selectionfileoption.h +++ b/src/gromacs/selection/selectionfileoption.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2012, by the GROMACS development team, led by + * Copyright (c) 2012,2014, 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. @@ -76,7 +76,8 @@ class SelectionFileOption : public AbstractOption explicit SelectionFileOption(const char *name); private: - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; }; /*! \libinternal \brief @@ -94,13 +95,6 @@ class SelectionFileOptionInfo : public OptionInfo * Does not throw. */ explicit SelectionFileOptionInfo(SelectionFileOptionStorage *option); - - //! \copydoc SelectionOptionInfo::setManager() - void setManager(SelectionOptionManager *manager); - - private: - SelectionFileOptionStorage &option(); - const SelectionFileOptionStorage &option() const; }; } // namespace gmx diff --git a/src/gromacs/selection/selectionfileoptionstorage.h b/src/gromacs/selection/selectionfileoptionstorage.h index a19321962f..ac628bf7ba 100644 --- a/src/gromacs/selection/selectionfileoptionstorage.h +++ b/src/gromacs/selection/selectionfileoptionstorage.h @@ -63,29 +63,25 @@ class SelectionFileOptionStorage : public AbstractOptionStorage * Initializes the storage from option settings. * * \param[in] settings Storage settings. + * \param manager Manager for this object. */ - SelectionFileOptionStorage(const SelectionFileOption &settings); + SelectionFileOptionStorage(const SelectionFileOption &settings, + SelectionOptionManager *manager); virtual OptionInfo &optionInfo() { return info_; } virtual std::string typeString() const { return "file"; } virtual int valueCount() const { return 0; } virtual std::string formatValue(int /*i*/) const { return ""; } - //! \copydoc SelectionFileOptionInfo::setManager() - void setManager(SelectionOptionManager *manager) - { - manager_ = manager; - } - private: virtual void clearSet(); virtual void convertValue(const std::string &value); virtual void processSet(); virtual void processAll() {} - SelectionFileOptionInfo info_; - SelectionOptionManager *manager_; - bool bValueParsed_; + SelectionFileOptionInfo info_; + SelectionOptionManager &manager_; + bool bValueParsed_; }; } // namespace gmx diff --git a/src/gromacs/selection/selectionoption.cpp b/src/gromacs/selection/selectionoption.cpp index 41da1f5de0..62fbc9cf50 100644 --- a/src/gromacs/selection/selectionoption.cpp +++ b/src/gromacs/selection/selectionoption.cpp @@ -46,6 +46,7 @@ #include +#include "gromacs/options/optionmanagercontainer.h" #include "gromacs/selection/selection.h" #include "gromacs/selection/selectionoptionmanager.h" #include "gromacs/utility/exceptions.h" @@ -59,26 +60,18 @@ namespace gmx * SelectionOptionStorage */ -SelectionOptionStorage::SelectionOptionStorage(const SelectionOption &settings) +SelectionOptionStorage::SelectionOptionStorage(const SelectionOption &settings, + SelectionOptionManager *manager) : MyBase(settings, OptionFlags() | efOption_NoDefaultValue | efOption_DontCheckMinimumCount), - info_(this), manager_(NULL), defaultText_(settings.defaultText_), + info_(this), manager_(*manager), defaultText_(settings.defaultText_), selectionFlags_(settings.selectionFlags_) { + GMX_RELEASE_ASSERT(manager != NULL, + "SelectionOptionManager must be added before SelectionOption"); GMX_RELEASE_ASSERT(!hasFlag(efOption_MultipleTimes), "allowMultiple() is not supported for selection options"); -} - - -void SelectionOptionStorage::setManager(SelectionOptionManager *manager) -{ - GMX_RELEASE_ASSERT(manager_ == NULL || manager_ == manager, - "Manager cannot be changed once set"); - if (manager_ == NULL) - { - manager->registerOption(this); - manager_ = manager; - } + manager_.registerOption(this); } @@ -124,18 +117,14 @@ void SelectionOptionStorage::addSelections( void SelectionOptionStorage::convertValue(const std::string &value) { - GMX_RELEASE_ASSERT(manager_ != NULL, "Manager is not set"); - - manager_->convertOptionValue(this, value, false); + manager_.convertOptionValue(this, value, false); } void SelectionOptionStorage::processSetValues(ValueList *values) { - GMX_RELEASE_ASSERT(manager_ != NULL, "Manager is not set"); - if (values->size() == 0) { - manager_->requestOptionDelayedParsing(this); + manager_.requestOptionDelayedParsing(this); } else if (values->size() < static_cast(minValueCount())) { @@ -145,14 +134,13 @@ void SelectionOptionStorage::processSetValues(ValueList *values) void SelectionOptionStorage::processAll() { - GMX_RELEASE_ASSERT(manager_ != NULL, "Manager is not set"); if (!isSet() && !defaultText_.empty()) { - manager_->convertOptionValue(this, defaultText_, true); + manager_.convertOptionValue(this, defaultText_, true); } if (isRequired() && !isSet()) { - manager_->requestOptionDelayedParsing(this); + manager_.requestOptionDelayedParsing(this); markAsSet(); } } @@ -227,11 +215,6 @@ const SelectionOptionStorage &SelectionOptionInfo::option() const return static_cast(OptionInfo::option()); } -void SelectionOptionInfo::setManager(SelectionOptionManager *manager) -{ - option().setManager(manager); -} - void SelectionOptionInfo::setValueCount(int count) { option().setAllowedValueCount(count); @@ -267,9 +250,12 @@ void SelectionOptionInfo::setDynamicMask(bool bEnabled) * SelectionOption */ -AbstractOptionStoragePointer SelectionOption::createStorage() const +AbstractOptionStoragePointer +SelectionOption::createStorage(const OptionManagerContainer &managers) const { - return AbstractOptionStoragePointer(new SelectionOptionStorage(*this)); + return AbstractOptionStoragePointer( + new SelectionOptionStorage( + *this, managers.get())); } @@ -277,11 +263,14 @@ AbstractOptionStoragePointer SelectionOption::createStorage() const * SelectionFileOptionStorage */ -SelectionFileOptionStorage::SelectionFileOptionStorage(const SelectionFileOption &settings) +SelectionFileOptionStorage::SelectionFileOptionStorage( + const SelectionFileOption &settings, SelectionOptionManager *manager) : AbstractOptionStorage(settings, OptionFlags() | efOption_MultipleTimes | efOption_DontCheckMinimumCount), - info_(this), manager_(NULL), bValueParsed_(false) + info_(this), manager_(*manager), bValueParsed_(false) { + GMX_RELEASE_ASSERT(manager != NULL, + "SelectionOptionManager must be added before SelectionFileOption"); } void SelectionFileOptionStorage::clearSet() @@ -291,15 +280,13 @@ void SelectionFileOptionStorage::clearSet() void SelectionFileOptionStorage::convertValue(const std::string &value) { - GMX_RELEASE_ASSERT(manager_ != NULL, "Manager is not set"); - if (bValueParsed_) { GMX_THROW(InvalidInputError("More than one file name provided")); } bValueParsed_ = true; // TODO: Should we throw an InvalidInputError if the file does not exist? - manager_->parseRequestedFromFile(value); + manager_.parseRequestedFromFile(value); } void SelectionFileOptionStorage::processSet() @@ -320,21 +307,6 @@ SelectionFileOptionInfo::SelectionFileOptionInfo(SelectionFileOptionStorage *opt { } -SelectionFileOptionStorage &SelectionFileOptionInfo::option() -{ - return static_cast(OptionInfo::option()); -} - -const SelectionFileOptionStorage &SelectionFileOptionInfo::option() const -{ - return static_cast(OptionInfo::option()); -} - -void SelectionFileOptionInfo::setManager(SelectionOptionManager *manager) -{ - option().setManager(manager); -} - /******************************************************************** * SelectionFileOption @@ -346,9 +318,12 @@ SelectionFileOption::SelectionFileOption(const char *name) setDescription("Provide selections from files"); } -AbstractOptionStoragePointer SelectionFileOption::createStorage() const +AbstractOptionStoragePointer +SelectionFileOption::createStorage(const OptionManagerContainer &managers) const { - return AbstractOptionStoragePointer(new SelectionFileOptionStorage(*this)); + return AbstractOptionStoragePointer( + new SelectionFileOptionStorage( + *this, managers.get())); } } // namespace gmx diff --git a/src/gromacs/selection/selectionoption.h b/src/gromacs/selection/selectionoption.h index cba014a055..a67e4e337b 100644 --- a/src/gromacs/selection/selectionoption.h +++ b/src/gromacs/selection/selectionoption.h @@ -60,6 +60,10 @@ class SelectionOptionStorage; * * Public methods in this class do not throw. * + * To use options of this type, SelectionOptionManager must first be added to + * the Options collection. For trajectory analysis tools, the framework takes + * care of this. + * * \todo * Support for specifying that an option accepts, e.g., two to four selections. * Currently, only a fixed count or any number of selections is possible. @@ -145,7 +149,8 @@ class SelectionOption : public OptionTemplate using MyBase::defaultValue; using MyBase::defaultValueIfSet; - virtual AbstractOptionStoragePointer createStorage() const; + virtual AbstractOptionStoragePointer createStorage( + const OptionManagerContainer &managers) const; const char *defaultText_; SelectionFlags selectionFlags_; @@ -211,22 +216,6 @@ class SelectionOptionInfo : public OptionInfo */ explicit SelectionOptionInfo(SelectionOptionStorage *option); - /*! \brief - * Set manager for handling interaction with other options and the - * selection collection. - * - * \param manager Selection manager to set. - * - * This must be called before the values are added. - * - * Typically it is called through setManagerForSelectionOptions(), - * which recursively sets the manager for all selection options in - * an Options object. - * - * Does not throw. - */ - void setManager(SelectionOptionManager *manager); - /*! \brief * Sets the number of selections allowed for the option. * diff --git a/src/gromacs/selection/selectionoptionmanager.cpp b/src/gromacs/selection/selectionoptionmanager.cpp index 3600bd2923..effb758ba0 100644 --- a/src/gromacs/selection/selectionoptionmanager.cpp +++ b/src/gromacs/selection/selectionoptionmanager.cpp @@ -43,7 +43,6 @@ #include -#include "gromacs/options/optionsvisitor.h" #include "gromacs/selection/selection.h" #include "gromacs/selection/selectioncollection.h" #include "gromacs/selection/selectionoption.h" @@ -331,60 +330,4 @@ SelectionOptionManager::parseRequestedFromString(const std::string &str) impl_->placeSelectionsInRequests(selections); } -/******************************************************************** - * Global functions - */ - -namespace -{ - -/*! \internal \brief - * Visitor that sets the manager for each selection option. - * - * \ingroup module_selection - */ -class SelectionOptionManagerSetter : public OptionsModifyingVisitor -{ - public: - //! Construct a visitor that sets given manager. - explicit SelectionOptionManagerSetter(SelectionOptionManager *manager) - : manager_(manager) - { - } - - void visitSubSection(Options *section) - { - OptionsModifyingIterator iterator(section); - iterator.acceptSubSections(this); - iterator.acceptOptions(this); - } - - void visitOption(OptionInfo *option) - { - SelectionOptionInfo *selOption - = option->toType(); - if (selOption != NULL) - { - selOption->setManager(manager_); - } - SelectionFileOptionInfo *selFileOption - = option->toType(); - if (selFileOption != NULL) - { - selFileOption->setManager(manager_); - } - } - - private: - SelectionOptionManager *manager_; -}; - -} // namespace - -void setManagerForSelectionOptions(Options *options, - SelectionOptionManager *manager) -{ - SelectionOptionManagerSetter(manager).visitSubSection(options); -} - } // namespace gmx diff --git a/src/gromacs/selection/selectionoptionmanager.h b/src/gromacs/selection/selectionoptionmanager.h index 8545661bbf..ec26193b58 100644 --- a/src/gromacs/selection/selectionoptionmanager.h +++ b/src/gromacs/selection/selectionoptionmanager.h @@ -45,6 +45,7 @@ #include +#include "../options/options.h" #include "../utility/common.h" namespace gmx @@ -62,6 +63,8 @@ class SelectionOptionStorage; * require actions outside options parsing. * It also implements the coupling between SelectionOption and * SelectionFileOption. + * It needs to be added using Options::addManager() before SelectionOption or + * SelectionFileOption options can be added to an Options collection. * * The main features of this class are: * - convertOptionValue(), which is used to convert string values into @@ -73,12 +76,10 @@ class SelectionOptionStorage; * parseRequestedFromStdin(), parseRequestedFromFile() or * parseRequstedFromString(). * - * \see setManagerForSelectionOptions() - * * \inpublicapi * \ingroup module_selection */ -class SelectionOptionManager +class SelectionOptionManager : public OptionManagerInterface { public: /*! \brief @@ -87,7 +88,7 @@ class SelectionOptionManager * \throws std::bad_alloc if out of memory. */ explicit SelectionOptionManager(SelectionCollection *selections); - ~SelectionOptionManager(); + virtual ~SelectionOptionManager(); /*! \brief * Adds a selection option to be managed. @@ -209,20 +210,6 @@ class SelectionOptionManager friend class SelectionOptionStorage; }; -/*! \brief - * Set manager for all selection options. - * - * Recursively sets the manager to \p manager for all selection options in - * \p options. - * Must be called before value assignment starts for \p options. - * - * Does not throw. - * - * \inpublicapi - */ -void setManagerForSelectionOptions(Options *options, - SelectionOptionManager *manager); - } // namespace gmx #endif diff --git a/src/gromacs/selection/selectionoptionstorage.h b/src/gromacs/selection/selectionoptionstorage.h index 00e3622ba8..fb9300d2b1 100644 --- a/src/gromacs/selection/selectionoptionstorage.h +++ b/src/gromacs/selection/selectionoptionstorage.h @@ -69,16 +69,15 @@ class SelectionOptionStorage : public OptionStorageTemplate * Initializes the storage from option settings. * * \param[in] settings Storage settings. + * \param manager Manager for this object. */ - SelectionOptionStorage(const SelectionOption &settings); + SelectionOptionStorage(const SelectionOption &settings, + SelectionOptionManager *manager); virtual OptionInfo &optionInfo() { return info_; } virtual std::string typeString() const { return "selection"; } virtual std::string formatSingleValue(const Selection &value) const; - //! \copydoc SelectionOptionInfo::setManager() - void setManager(SelectionOptionManager *manager); - /*! \brief * Adds selections to the storage. * @@ -127,7 +126,7 @@ class SelectionOptionStorage : public OptionStorageTemplate virtual void processAll(); SelectionOptionInfo info_; - SelectionOptionManager *manager_; + SelectionOptionManager &manager_; std::string defaultText_; SelectionFlags selectionFlags_; }; diff --git a/src/gromacs/selection/tests/selectionoption.cpp b/src/gromacs/selection/tests/selectionoption.cpp index 4c46400690..dc5c3ec799 100644 --- a/src/gromacs/selection/tests/selectionoption.cpp +++ b/src/gromacs/selection/tests/selectionoption.cpp @@ -69,7 +69,6 @@ class SelectionOptionTestBase : public ::testing::Test public: SelectionOptionTestBase(); - void setManager(); void loadTopology(const char *filename); gmx::SelectionCollection sc_; @@ -83,15 +82,11 @@ class SelectionOptionTestBase : public ::testing::Test SelectionOptionTestBase::SelectionOptionTestBase() : manager_(&sc_), options_(NULL, NULL) { + options_.addManager(&manager_); sc_.setReferencePosType("atom"); sc_.setOutputPosType("atom"); } -void SelectionOptionTestBase::setManager() -{ - setManagerForSelectionOptions(&options_, &manager_); -} - void SelectionOptionTestBase::loadTopology(const char *filename) { topManager_.loadTopology(filename); @@ -112,7 +107,6 @@ TEST_F(SelectionOptionTest, ParsesSimpleSelection) gmx::Selection sel; using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption(SelectionOption("sel").store(&sel))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -132,7 +126,6 @@ TEST_F(SelectionOptionTest, HandlesDynamicSelectionWhenStaticRequired) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(&sel).onlyStatic())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -150,7 +143,6 @@ TEST_F(SelectionOptionTest, HandlesNonAtomicSelectionWhenAtomsRequired) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(&sel).onlyAtoms())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -171,7 +163,6 @@ TEST_F(SelectionOptionTest, ChecksEmptySelections) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(&sel))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -191,7 +182,6 @@ TEST_F(SelectionOptionTest, ChecksEmptyDelayedSelections) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(&sel))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -210,7 +200,6 @@ TEST_F(SelectionOptionTest, HandlesTooManySelections) gmx::Selection sel; using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption(SelectionOption("sel").store(&sel))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -229,7 +218,6 @@ TEST_F(SelectionOptionTest, HandlesTooFewSelections) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(sel).valueCount(2))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -247,7 +235,6 @@ TEST_F(SelectionOptionTest, HandlesDefaultSelectionText) using gmx::SelectionOption; options_.addOption(SelectionOption("sel").store(&sel) .defaultSelectionText("all")); - setManager(); EXPECT_NO_THROW_GMX(options_.finish()); @@ -266,7 +253,6 @@ TEST_F(SelectionOptionTest, HandlesAdjuster) using gmx::SelectionOption; gmx::SelectionOptionInfo *info = options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue()); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -286,7 +272,6 @@ TEST_F(SelectionOptionTest, HandlesDynamicWhenStaticRequiredWithAdjuster) using gmx::SelectionOption; gmx::SelectionOptionInfo *info = options_.addOption( SelectionOption("sel").store(&sel)); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -305,7 +290,6 @@ TEST_F(SelectionOptionTest, HandlesTooManySelectionsWithAdjuster) using gmx::SelectionOption; gmx::SelectionOptionInfo *info = options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue()); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -325,7 +309,6 @@ TEST_F(SelectionOptionTest, HandlesTooFewSelectionsWithAdjuster) using gmx::SelectionOption; gmx::SelectionOptionInfo *info = options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue()); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -344,7 +327,6 @@ TEST_F(SelectionOptionTest, HandlesDelayedRequiredSelection) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(&sel).required())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -362,7 +344,6 @@ TEST_F(SelectionOptionTest, HandlesTooFewDelayedRequiredSelections) ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").store(sel).required() .valueCount(2))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -377,7 +358,6 @@ TEST_F(SelectionOptionTest, HandlesDelayedOptionalSelection) gmx::Selection sel; using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption(SelectionOption("sel").store(&sel))); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -396,7 +376,6 @@ TEST_F(SelectionOptionTest, HandlesDelayedSelectionWithAdjuster) using gmx::SelectionOption; gmx::SelectionOptionInfo *info = options_.addOption( SelectionOption("sel").storeVector(&sel).valueCount(3)); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -435,7 +414,6 @@ TEST_F(SelectionFileOptionTest, HandlesSingleSelectionOptionFromFile) ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("reqsel").storeVector(&reqsel) .multiValue().required())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -464,7 +442,6 @@ TEST_F(SelectionFileOptionTest, HandlesTwoSeparateSelectionOptions) SelectionOption("sel1").storeVector(&sel1).multiValue())); ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel2").storeVector(&sel2).multiValue())); - setManager(); gmx::OptionsAssigner assigner(&options_); std::string value(TestFileManager::getInputFilePath("selfile.dat")); @@ -501,7 +478,6 @@ TEST_F(SelectionFileOptionTest, HandlesTwoSelectionOptionsFromSingleFile) SelectionOption("sel1").storeVector(&sel1))); ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel2").storeVector(&sel2))); - setManager(); gmx::OptionsAssigner assigner(&options_); std::string value(TestFileManager::getInputFilePath("selfile.dat")); @@ -535,7 +511,6 @@ TEST_F(SelectionFileOptionTest, HandlesRequiredOptionFromFile) ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("optsel").storeVector(&optsel) .multiValue())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -569,7 +544,6 @@ TEST_F(SelectionFileOptionTest, HandlesRequiredOptionFromFileWithOtherOptionSet) ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel2").storeVector(&sel2) .multiValue().required())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -600,7 +574,6 @@ TEST_F(SelectionFileOptionTest, HandlesTwoRequiredOptionsFromSingleFile) SelectionOption("sel1").storeVector(&sel1).required())); ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel2").storeVector(&sel2).required())); - setManager(); gmx::OptionsAssigner assigner(&options_); std::string value(TestFileManager::getInputFilePath("selfile.dat")); @@ -625,7 +598,6 @@ TEST_F(SelectionFileOptionTest, GivesErrorWithNoFile) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -644,7 +616,6 @@ TEST_F(SelectionFileOptionTest, GivesErrorWithNonExistentFile) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); @@ -667,7 +638,6 @@ TEST_F(SelectionFileOptionTest, GivesErrorWithMultipleFiles) using gmx::SelectionOption; ASSERT_NO_THROW_GMX(options_.addOption( SelectionOption("sel").storeVector(&sel).multiValue())); - setManager(); gmx::OptionsAssigner assigner(&options_); EXPECT_NO_THROW_GMX(assigner.start()); diff --git a/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp b/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp index b7165f94bf..d80bc883ae 100644 --- a/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp +++ b/src/gromacs/trajectoryanalysis/cmdlinerunner.cpp @@ -108,20 +108,20 @@ TrajectoryAnalysisCommandLineRunner::Impl::parseOptions( SelectionCollection *selections, int *argc, char *argv[]) { - Options options(NULL, NULL); - Options moduleOptions(module_->name(), module_->description()); - Options commonOptions("common", "Common analysis control"); - Options selectionOptions("selection", "Common selection control"); - module_->initOptions(&moduleOptions, settings); - common->initOptions(&commonOptions); - selections->initOptions(&selectionOptions); + 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(&seloptManager); options.addSubSection(&commonOptions); options.addSubSection(&selectionOptions); options.addSubSection(&moduleOptions); - SelectionOptionManager seloptManager(selections); - setManagerForSelectionOptions(&options, &seloptManager); + module_->initOptions(&moduleOptions, settings); + common->initOptions(&commonOptions); + selections->initOptions(&selectionOptions); { CommandLineParser parser(&options); @@ -254,21 +254,20 @@ TrajectoryAnalysisCommandLineRunner::writeHelp(const CommandLineHelpContext &con TrajectoryAnalysisSettings settings; TrajectoryAnalysisRunnerCommon common(&settings); - Options options(NULL, NULL); - Options moduleOptions(impl_->module_->name(), impl_->module_->description()); - Options commonOptions("common", "Common analysis control"); - Options selectionOptions("selection", "Common selection control"); - - impl_->module_->initOptions(&moduleOptions, &settings); - common.initOptions(&commonOptions); - selections.initOptions(&selectionOptions); + 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); - SelectionOptionManager seloptManager(&selections); - setManagerForSelectionOptions(&options, &seloptManager); + impl_->module_->initOptions(&moduleOptions, &settings); + common.initOptions(&commonOptions); + selections.initOptions(&selectionOptions); CommandLineHelpWriter(options) .setShowDescriptions(true) -- 2.22.0