Add basic grouping to Options
authorTeemu Murtola <teemu.murtola@gmail.com>
Sun, 19 Jul 2015 06:44:06 +0000 (09:44 +0300)
committerTeemu Murtola <teemu.murtola@gmail.com>
Sat, 25 Jul 2015 09:48:02 +0000 (11:48 +0200)
- 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

12 files changed:
src/gromacs/commandline/cmdlineparser.cpp
src/gromacs/commandline/tests/cmdlinehelpwriter.cpp
src/gromacs/commandline/tests/refdata/CommandLineHelpWriterTest_HandlesOptionGroups.xml [new file with mode: 0644]
src/gromacs/options/abstractoption.h
src/gromacs/options/options-impl.h
src/gromacs/options/options.cpp
src/gromacs/options/options.h
src/gromacs/options/optionsassigner.cpp
src/gromacs/options/optionsassigner.h
src/gromacs/options/optionsvisitor.cpp
src/gromacs/options/tests/optionsassigner.cpp
src/gromacs/trajectoryanalysis/cmdlinerunner.cpp

index 61f69d69f75b8a5047eaa8642cd372afb3681845..db654a1f0bac0114dee44ad043300f9bb9fa7ea8 100644 (file)
@@ -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
index 886b0b0d18f8f95168d2b8cdb6d0a294c2eda5db..49b674c699d0ae0242f702b12cea205b92eab7bf 100644 (file)
@@ -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 (file)
index 0000000..9e1a595
--- /dev/null
@@ -0,0 +1,23 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="referencedata.xsl"?>
+<ReferenceData>
+  <String Name="HelpText"><![CDATA[
+SYNOPSIS
+
+gmx [-main <int>] [-sub11 <int>] [-sub12 <int>] [-sub2 <int>]
+
+OPTIONS
+
+Other options:
+
+ -main   <int>
+           Option in root group
+ -sub11  <int>
+           Option in group 1
+ -sub12  <int>
+           Option in group 1
+ -sub2   <int>
+           Option in group 2
+
+]]></String>
+</ReferenceData>
index aa5f2fb1e5b58e92d142c4a77effd1677501e974..563c3621f68dd9a9ea0ce53083def578bbd2a186 100644 (file)
@@ -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 <typename T> 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
index a5294c04209ee8fa217cd93071da93a174f47dad..aaf61fb4d822ecfb3b6be2152b04b35f546a3a9a 100644 (file)
@@ -42,6 +42,8 @@
 #ifndef GMX_OPTIONS_OPTIONS_IMPL_H
 #define GMX_OPTIONS_OPTIONS_IMPL_H
 
+#include <list>
+#include <map>
 #include <string>
 #include <vector>
 
@@ -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<AbstractOptionStorage *> OptionList;
+                //! Convenience typedef for list of subgroups.
+                typedef std::list<Group> 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<AbstractOptionStorage>::type
             AbstractOptionStoragePointer;
         //! Convenience type for list of sections.
         typedef std::vector<Options *> SubSectionList;
-        //! Convenience type for list of options.
-        typedef std::vector<AbstractOptionStoragePointer> OptionList;
+        //! Convenience typedef for a map that contains all the options.
+        typedef std::map<std::string, AbstractOptionStoragePointer> 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
index 9b422c96404ec531809c7310f6873fedcf735f3c..97dabb99d699a9cd26528811762ceaaeb8eceef6 100644 (file)
@@ -43,6 +43,8 @@
 
 #include "options.h"
 
+#include <utility>
+
 #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<OptionMap::iterator, bool> 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 &section = **j;
index b312218a56bbb12a611cf03a8e6e7adc9676a737..67fcf257375cc7c86d986f408999047d0b32de6b 100644 (file)
@@ -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> impl_;
+        PrivateImplPointer<internal::OptionsImpl> 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.
index df3f1320d89da7ddb54996b610e7c5885d30a614..2842655c54f438b38559d32d2989383ec69d1b39 100644 (file)
@@ -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    = &currentSection();
-    Options               *oldRoot = NULL;
-    int                    upcount = 0;
-    std::deque<Options *>  searchList;
-    searchList.push_back(root);
-    while (option == NULL && !searchList.empty())
+    const Options         &section = 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<Options *> sections;
-        while (section != &currentSection())
-        {
-            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");
 }
 
index 3d06fd2095ed3b15593fe3f0c1a90cced3406155..35bb70dce9225e3e5b71146ef832a3df43086011 100644 (file)
@@ -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.
index 814ae734fdb2e19efa27e985ac0163ffb26b039c..c9584dee9eceb29528793f81022848cd94cd2e2c 100644 (file)
@@ -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.
 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 <class VisitorType>
+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
index cf9648cc1cb269251fa421740ab36e7a0bfff30b..0925a4900ae75a413d8ac2104852cba247c311a6 100644 (file)
@@ -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)
index 5ac15b6281f04b4386ebe8ea77f95d6855d4a70f..a32a591e356d58f16d7d121d2db48f7244b90fea 100644 (file)
@@ -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())