Made option value assignment use transactions.
authorTeemu Murtola <teemu.murtola@cbr.su.se>
Sat, 30 Jul 2011 16:27:18 +0000 (19:27 +0300)
committerTeemu Murtola <teemu.murtola@cbr.su.se>
Mon, 1 Aug 2011 17:42:28 +0000 (20:42 +0300)
It's now more predictable what will happen when option assignment fails:
if assignment of a set of values fails, the whole set is rolled back.
Within a set, there are still cases where the state may be different
after error recovery than it was before the erroneous values were added
(if value conversion may create more than one value from a single
value), but these could also be handled if they become issues.

src/gromacs/options/abstractoption.cpp
src/gromacs/options/abstractoptionstorage.h
src/gromacs/options/basicoptions.cpp
src/gromacs/options/basicoptionstorage.h
src/gromacs/options/optionflags.h
src/gromacs/options/optionstoragetemplate.h
src/gromacs/options/tests/abstractoptionstorage.cpp
src/gromacs/options/tests/optionsassigner.cpp
src/gromacs/selection/selectionoption.cpp
src/gromacs/selection/selectionoptionstorage.h

index 1a831085cc43ca35eb51484759e06e8f61b091f4..57e5d2bdf4be2066edef8c6c50be6c05eb675c5e 100644 (file)
@@ -55,7 +55,7 @@ AbstractOptionStorage::AbstractOptionStorage(const AbstractOption &settings,
     : _flags(settings._flags | staticFlags),
       _minValueCount(settings._minValueCount),
       _maxValueCount(settings._maxValueCount),
-      _currentValueCount(-1),
+      _inSet(false),
       _options(options)
 {
     // If the maximum number of values is not known, storage to
@@ -88,67 +88,36 @@ void AbstractOptionStorage::startSource()
 
 void AbstractOptionStorage::startSet()
 {
-    if (hasFlag(efHasDefaultValue))
-    {
-        clearFlag(efHasDefaultValue);
-        clear();
-    }
-    else if (isSet() && !hasFlag(efMulti))
+    GMX_RELEASE_ASSERT(!_inSet, "finishSet() not called");
+    clearSet();
+    // The last condition takes care of the situation where multiple
+    // sources are used, and a later source should be able to reassign
+    // the value even though the option is already set.
+    if (isSet() && !hasFlag(efMulti) && !hasFlag(efHasDefaultValue))
     {
         GMX_THROW(InvalidInputError("Option specified multiple times"));
     }
-    _currentValueCount = 0;
+    _inSet = true;
 }
 
 void AbstractOptionStorage::appendValue(const std::string &value)
 {
-    GMX_RELEASE_ASSERT(_currentValueCount >= 0, "startSet() not called");
-    if (!hasFlag(efConversionMayNotAddValues) && _maxValueCount >= 0
-        && _currentValueCount >= _maxValueCount)
-    {
-        GMX_THROW(InvalidInputError("Ignoring extra value: " + value));
-    }
+    GMX_RELEASE_ASSERT(_inSet, "startSet() not called");
     convertValue(value);
 }
 
-class CurrentCountClearer
-{
-    public:
-        explicit CurrentCountClearer(int *currentValueCount)
-            : value_(currentValueCount)
-        {
-        }
-        ~CurrentCountClearer()
-        {
-            *value_ = -1;
-        }
-
-    private:
-        int                    *value_;
-};
-
 void AbstractOptionStorage::finishSet()
 {
-    GMX_RELEASE_ASSERT(_currentValueCount >= 0, "startSet() not called");
-
+    GMX_RELEASE_ASSERT(_inSet, "startSet() not called");
+    _inSet = false;
     setFlag(efSet);
-
-    CurrentCountClearer clearOnExit(&_currentValueCount);
-    // TODO: Remove invalid values
-    processSet(_currentValueCount);
-    // TODO: Should this also be checked if processSet throws?
-    if (!hasFlag(efDontCheckMinimumCount)
-        && _currentValueCount < _minValueCount)
-    {
-        GMX_THROW(InvalidInputError("Too few (valid) values"));
-    }
+    processSet();
 }
 
 void AbstractOptionStorage::finish()
 {
-    GMX_RELEASE_ASSERT(_currentValueCount == -1, "finishSet() not called");
+    GMX_RELEASE_ASSERT(!_inSet, "finishSet() not called");
     processAll();
-    // TODO: Should this also be checked if processAll throws?
     if (hasFlag(efRequired) && !isSet())
     {
         GMX_THROW(InvalidInputError("Option is required, but not set"));
@@ -180,17 +149,4 @@ void AbstractOptionStorage::setMaxValueCount(int count)
     }
 }
 
-void AbstractOptionStorage::incrementValueCount()
-{
-    if (_currentValueCount == -1)
-    {
-        return;
-    }
-    if (_maxValueCount >= 0 && _currentValueCount >= _maxValueCount)
-    {
-        GMX_THROW(InvalidInputError("Too many values"));
-    }
-    ++_currentValueCount;
-}
-
 } // namespace gmx
index 73e460e6dea8d0e291cc3874eebda6c47a486729..c608416a39709b62fdec9c825444a6f4b1c44f1d 100644 (file)
@@ -203,12 +203,12 @@ class AbstractOptionStorage
         const Options &hostOptions() const { return *_options; }
 
         /*! \brief
-         * Removes all values from the storage.
+         * Removes all values from temporary storage for a set.
          *
-         * This function is called also before the first value is added,
-         * allowing the storage to set a default value in initialization.
+         * This function is always called before starting to add values to
+         * a set, allowing the storage to clear its internal buffers.
          */
-        virtual void clear() = 0;
+        virtual void clearSet() = 0;
         /*! \brief
          * Adds a new value, converting it from a string.
          *
@@ -222,13 +222,10 @@ class AbstractOptionStorage
          * Performs validation and/or actions once a set of values has been
          * added.
          *
-         * \param[in] nvalues  Number of values added since the previous call
-         *      to finishSet().
-         *
          * This function may be called multiple times of the underlying option
          * can be specified multiple times.
          */
-        virtual void processSet(int nvalues) = 0;
+        virtual void processSet() = 0;
         /*! \brief
          * Performs validation and/or actions once all values have been added.
          *
@@ -236,11 +233,6 @@ class AbstractOptionStorage
          */
         virtual void processAll() = 0;
 
-        /*! \brief
-         * Increments the number of values for the current set.
-         */
-        void incrementValueCount();
-
     private:
         std::string             _name;
         std::string             _descr;
@@ -250,8 +242,8 @@ class AbstractOptionStorage
         int                     _minValueCount;
         //! Maximum allowed number of values (in one set), or -1 if no limit.
         int                     _maxValueCount;
-        //! Number of values added so far to the current set, or -1 if not in one.
-        int                     _currentValueCount;
+        //! Whether we are currently assigning values to a set.
+        bool                    _inSet;
         //! Parent Options object.
         Options                *_options;
 
index e1592ddf9c9471371d47c7235e703d061c1a1524..c3f1c52605e337eef7f593f8519b5ad16c5dd47d 100644 (file)
 #include "basicoptionstorage.h"
 
 template <typename T> static
-void expandVector(int length, int *nvalues, std::vector<T> *values)
+void expandVector(size_t length, std::vector<T> *values)
 {
-    if (length > 0 && *nvalues > 0 && *nvalues != length)
+    if (length > 0 && values->size() > 0 && values->size() != length)
     {
-        if (*nvalues != 1)
+        if (values->size() != 1)
         {
-            values->resize(values->size() - *nvalues);
             GMX_THROW(gmx::InvalidInputError(gmx::formatString(
-                      "Expected 1 or %d values, got %d", length, *nvalues)));
+                      "Expected 1 or %d values, got %d", length, values->size())));
         }
-        const T &value = (*values)[values->size() - 1];
-        values->resize(values->size() + length - 1, value);
-        *nvalues = length;
+        const T &value = (*values)[0];
+        values->resize(length, value);
     }
 }
 
@@ -128,13 +126,12 @@ void IntegerOptionStorage::convertValue(const std::string &value)
     addValue(ival);
 }
 
-void IntegerOptionStorage::processSet(int nvalues)
+void IntegerOptionStorage::processSetValues(ValueList *values)
 {
     if (hasFlag(efVector))
     {
-        expandVector(maxValueCount(), &nvalues, &values());
+        expandVector(maxValueCount(), values);
     }
-    MyBase::processSet(nvalues);
 }
 
 /********************************************************************
@@ -188,13 +185,12 @@ void DoubleOptionStorage::convertValue(const std::string &value)
     addValue(dval);
 }
 
-void DoubleOptionStorage::processSet(int nvalues)
+void DoubleOptionStorage::processSetValues(ValueList *values)
 {
     if (hasFlag(efVector))
     {
-        expandVector(maxValueCount(), &nvalues, &values());
+        expandVector(maxValueCount(), values);
     }
-    MyBase::processSet(nvalues);
 }
 
 void DoubleOptionStorage::processAll()
@@ -207,8 +203,8 @@ void DoubleOptionStorage::processAll()
         {
             (*i) *= factor;
         }
+        refreshValues();
     }
-    MyBase::processAll();
 }
 
 /********************************************************************
@@ -281,10 +277,7 @@ StringOptionStorage::StringOptionStorage(const StringOption &settings, Options *
     {
         clear();
         addValue(_allowed[settings._defaultEnumIndex]);
-        if (_enumIndexStore != NULL)
-        {
-            *_enumIndexStore = settings._defaultEnumIndex;
-        }
+        commitValues();
     }
 }
 
@@ -319,9 +312,19 @@ void StringOptionStorage::convertValue(const std::string &value)
             GMX_THROW(InvalidInputError("Invalid value: " + value));
         }
         addValue(*match);
-        if (_enumIndexStore != NULL)
+    }
+}
+
+void StringOptionStorage::refreshValues()
+{
+    MyBase::refreshValues();
+    if (_enumIndexStore != NULL)
+    {
+        for (size_t i = 0; i < values().size(); ++i)
         {
-            _enumIndexStore[valueCount() - 1] = (match - _allowed.begin());
+            ValueList::const_iterator match =
+                std::find(_allowed.begin(), _allowed.end(), values()[i]);
+            _enumIndexStore[i] = (match - _allowed.begin());
         }
     }
 }
index 96cb2a109330d22d03d58558ee0cc33b0c1bbb75..7f0f651a978791bdcd7d0fbe02dc7905b4b46b8a 100644 (file)
@@ -99,7 +99,7 @@ class IntegerOptionStorage : public OptionStorageTemplate<int>
 
     private:
         virtual void convertValue(const std::string &value);
-        virtual void processSet(int nvalues);
+        virtual void processSetValues(ValueList *values);
 };
 
 /*! \internal \brief
@@ -116,7 +116,7 @@ class DoubleOptionStorage : public OptionStorageTemplate<double>
 
     private:
         virtual void convertValue(const std::string &value);
-        virtual void processSet(int nvalues);
+        virtual void processSetValues(ValueList *values);
         virtual void processAll();
 
         bool                    _bTime;
@@ -136,6 +136,7 @@ class StringOptionStorage : public OptionStorageTemplate<std::string>
 
     private:
         virtual void convertValue(const std::string &value);
+        virtual void refreshValues();
 
         ValueList               _allowed;
         int                    *_enumIndexStore;
index b15ae3900f33f09872d0f9bf43100df82fadee1d..bcb7fd79a3d18a147b2c182e41f475805722d037 100644 (file)
@@ -90,23 +90,14 @@ enum OptionFlag
     efExternalValueVector = 1<<10,
     //! %Option does not support default values.
     efNoDefaultValue      = 1<<7,
-    /*! \brief
-     * Storage object may add zero values even when a value is provided.
-     *
-     * In order to do proper error checking, this flag should be set when it is
-     * possible that the AbstractOptionStorage::appendValue() method of the
-     * storage object does not add any values for the option and still
-     * succeeds.
-     */
-    efConversionMayNotAddValues = 1<<11,
     /*! \brief
      * Storage object does its custom checking for minimum value count.
      *
-     * If this flag is set, the class derived from AbstractOptionStorage should
-     * implement processSet(), processAll(), and possible other functions it
-     * provides such that it always fails if not enough values are provided.
+     * If this flag is set, the class derived from OptionStorageTemplate should
+     * implement processSetValues(), processAll(), and possible other functions
+     * it provides such that it always fails if not enough values are provided.
      * This is useful to override the default check, which is done in
-     * AbstractOptionStorage::processSet().
+     * OptionStorageTemplate::processSet().
      */
     efDontCheckMinimumCount     = 1<<16,
     efFile                = 1<<12,
index 3554560e42fd9ff64028e676f70cb001fdf015ce..e2c1324c25ddf72a2ec88df3ca54a02afaf2babc 100644 (file)
@@ -58,7 +58,7 @@ class Options;
  *
  * \tparam T Assignable type that stores a single option value.
  *
- * Provides an implementation of the clear() and valueCount() methods of
+ * Provides an implementation of the clearSet() and valueCount() methods of
  * AbstractOptionStorage, as well as a basic implementation of processSet() and
  * processAll().  This leaves typeString(), formatValue(), and convertValue()
  * to be implemented in derived classes.
@@ -96,34 +96,44 @@ class OptionStorageTemplate : public AbstractOptionStorage
                               OptionFlags staticFlags = OptionFlags());
 
 
-        virtual void clear();
+        virtual void clearSet();
         /*! \copydoc AbstractOptionStorage::convertValue()
          *
          * Derived classes should call addValue() after they have converted
          * \p value to the storage type.
          */
         virtual void convertValue(const std::string &value) = 0;
-        /*! \copydoc AbstractOptionStorage::processSet()
+        /*! \brief
+         * Processes values for a set after all have been converted.
+         *
+         * \param[in,out] values Valid values in the set.
          *
-         * The implementation in OptionStorageTemplate copies the values
-         * from the main storage vector to alternate locations, and always
-         * succeeds.  Derived classes should always call the base class
-         * implementation if they override this method.
+         * This method is called after all convertValue() calls for a set.
+         * \p values contains all values that were validly converted by
+         * convertValue().  The derived class may alter the values.
          */
-        virtual void processSet(int nvalues)
+        virtual void processSetValues(ValueList *values)
         {
-            processValues(nvalues);
         }
+        /*! \copydoc AbstractOptionStorage::processSet()
+         *
+         * OptionStorage template implements this method, and provides a more
+         * detailed processSetValues() method that can be overridden in
+         * subclasses.
+         */
+        virtual void processSet();
         /*! \copydoc AbstractOptionStorage::processAll()
          *
          * The implementation in OptionStorageTemplate does nothing.
-         * Derived classes should still always call the base class
-         * implementation if they override this method.
          */
         virtual void processAll()
         {
         }
 
+        /*! \brief
+         * Removes all values from the storage.
+         */
+        void clear() { _values->clear(); }
         /*! \brief
          * Adds a value to the storage.
          *
@@ -133,23 +143,19 @@ class OptionStorageTemplate : public AbstractOptionStorage
          *
          * Derived classes should call this function from the convertValue()
          * implementation to add converted values to the storage.
-         * It is only necessary to check the return value if addValue() is
-         * called more than once from one convertValue() invocation, or if
-         * ::efConversionMayNotAddValues is specified.
          */
         void addValue(const T &value);
         /*! \brief
-         * Store values in alternate locations.
-         *
-         * \param[in] nvalues  Number of values to process.
-         *
-         * Stores the last \p nvalues values added with addValue() to the
-         * alternate storage locations.
+         * Commits values added with addValue().
          *
          * Derived classes should call this method if they use addValue()
          * outside convertValue(), e.g., to set a default value.
          */
-        void processValues(int nvalues);
+        void commitValues();
+        /*! \brief
+         * Store values in alternate locations.
+         */
+        virtual void refreshValues();
 
         //! Provides derived classes access to the current list of values.
         ValueList &values() { return *_values; }
@@ -157,6 +163,10 @@ class OptionStorageTemplate : public AbstractOptionStorage
         const ValueList &values() const { return *_values; }
 
     private:
+        /*! \brief
+         * Vector for temporary storage of values before commitSet() is called.
+         */
+        ValueList               _setValues;
         /*! \brief
          * Vector for primary storage of option values.
          *
@@ -209,6 +219,8 @@ OptionStorageTemplate<T>::OptionStorageTemplate(const OptionTemplate<T, U> &sett
             _values->clear();
             addValue(*settings._defaultValue);
             setFlag(efHasDefaultValue);
+            // TODO: This is a bit hairy, as it indirectly calls a virtual function.
+            commitValues();
         }
         else if (!hasFlag(efExternalValueVector) && _store != NULL)
         {
@@ -241,43 +253,69 @@ OptionStorageTemplate<T>::~OptionStorageTemplate()
 
 
 template <typename T>
-void OptionStorageTemplate<T>::clear()
+void OptionStorageTemplate<T>::clearSet()
 {
-    _values->clear();
+    _setValues.clear();
 }
 
 
 template <typename T>
-void OptionStorageTemplate<T>::addValue(const T &value)
+void OptionStorageTemplate<T>::processSet()
 {
-    incrementValueCount();
-    _values->push_back(value);
-    if (_store != NULL)
+    processSetValues(&_setValues);
+    if (_setValues.empty() && _defaultValueIfSet != NULL)
     {
-        _store[_values->size() - 1] = value;
+        addValue(*_defaultValueIfSet);
     }
-    if (_countptr != NULL)
+    if (!hasFlag(efDontCheckMinimumCount)
+        && _setValues.size() < static_cast<size_t>(minValueCount()))
     {
-        *_countptr = _values->size();
+        clearSet();
+        GMX_THROW(InvalidInputError("Too few (valid) values"));
     }
+    commitValues();
 }
 
 
 template <typename T>
-void OptionStorageTemplate<T>::processValues(int nvalues)
+void OptionStorageTemplate<T>::addValue(const T &value)
 {
-    if (nvalues == 0 && _defaultValueIfSet != NULL)
+    if (maxValueCount() >= 0
+        && _setValues.size() >= static_cast<size_t>(maxValueCount()))
     {
-        addValue(*_defaultValueIfSet);
-        nvalues = 1;
+        GMX_THROW(InvalidInputError("Too many values"));
+    }
+    _setValues.push_back(value);
+}
+
+
+template <typename T>
+void OptionStorageTemplate<T>::commitValues()
+{
+    if (hasFlag(efHasDefaultValue))
+    {
+        _values->swap(_setValues);
+        clearFlag(efHasDefaultValue);
     }
+    else
+    {
+        _values->insert(_values->end(), _setValues.begin(), _setValues.end());
+    }
+    clearSet();
+    refreshValues();
+}
+
+
+template <typename T>
+void OptionStorageTemplate<T>::refreshValues()
+{
     if (_countptr != NULL)
     {
         *_countptr = _values->size();
     }
     if (_store != NULL)
     {
-        for (size_t i = _values->size() - nvalues; i < _values->size(); ++i)
+        for (size_t i = 0; i < _values->size(); ++i)
         {
             _store[i] = (*_values)[i];
         }
index 5eb8a2ccd5d74f7efd1b3fa459098acbc7504498..4eb48f9e2267824f7d5a7e11b9fd8670bad4e37d 100644 (file)
@@ -71,13 +71,6 @@ class MockOptionStorage : public gmx::OptionStorageTemplate<std::string>
          */
         MockOptionStorage(const MockOption &settings, gmx::Options *options);
 
-        /*! \brief
-         * Calls processAll() in the base class.
-         */
-        void processAllBase()
-        {
-            MyBase::processAll();
-        }
         /*! \brief
          * Calls addValue("dummy") in the base class.
          */
@@ -93,12 +86,13 @@ class MockOptionStorage : public gmx::OptionStorageTemplate<std::string>
             setFlag(gmx::efSet);
         }
         using MyBase::addValue;
+        using MyBase::commitValues;
 
         virtual const char *typeString() const { return "mock"; }
         virtual std::string formatValue(int /*i*/) const { return ""; }
 
         MOCK_METHOD1(convertValue, void(const std::string &value));
-        MOCK_METHOD1(processSet, void(int nvalues));
+        MOCK_METHOD1(processSetValues, void(ValueList *values));
         MOCK_METHOD0(processAll, void());
 };
 
@@ -116,9 +110,6 @@ class MockOption : public gmx::OptionTemplate<std::string, MockOption>
         {
         }
 
-        //! Sets the required flags to support storage that may not add values.
-        MyClass &mayNotAddValues()
-        { setFlag(gmx::efConversionMayNotAddValues); return me(); }
         //! Sets an output pointer to give access to the created storage object.
         MyClass &storageObject(MockOptionStorage **storagePtr)
         { _storagePtr = storagePtr; return me(); }
@@ -145,8 +136,6 @@ MockOptionStorage::MockOptionStorage(const MockOption &settings, gmx::Options *o
     using ::testing::WithArg;
     ON_CALL(*this, convertValue(_))
         .WillByDefault(WithArg<0>(Invoke(this, &MockOptionStorage::addValue)));
-    ON_CALL(*this, processAll())
-        .WillByDefault(Invoke(this, &MockOptionStorage::processAllBase));
 }
 
 namespace
@@ -171,7 +160,8 @@ TEST(AbstractOptionStorageTest, HandlesSetInFinish)
         using ::testing::InvokeWithoutArgs;
         EXPECT_CALL(*mock, processAll())
             .WillOnce(DoAll(InvokeWithoutArgs(mock, &MockOptionStorage::setOption),
-                            InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue)));
+                            InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue),
+                            InvokeWithoutArgs(mock, &MockOptionStorage::commitValues)));
     }
 
     gmx::OptionsAssigner assigner(&options);
@@ -193,17 +183,19 @@ TEST(AbstractOptionStorageTest, HandlesValueRemoval)
     std::vector<std::string>    values;
     MockOptionStorage          *mock;
     ASSERT_NO_THROW(options.addOption(
-                        MockOption("name").storageObject(&mock).mayNotAddValues()
+                        MockOption("name").storageObject(&mock)
                             .storeVector(&values).multiValue()));
 
     {
         ::testing::InSequence dummy;
+        using ::testing::ElementsAre;
+        using ::testing::Pointee;
         using ::testing::Return;
         EXPECT_CALL(*mock, convertValue("a"));
         EXPECT_CALL(*mock, convertValue("b"))
             .WillOnce(Return());
         EXPECT_CALL(*mock, convertValue("c"));
-        EXPECT_CALL(*mock, processSet(2));
+        EXPECT_CALL(*mock, processSetValues(Pointee(ElementsAre("a", "c"))));
         EXPECT_CALL(*mock, processAll());
     }
 
@@ -238,12 +230,14 @@ TEST(AbstractOptionStorageTest, HandlesValueAddition)
     {
         ::testing::InSequence dummy;
         using ::testing::DoAll;
+        using ::testing::ElementsAre;
         using ::testing::InvokeWithoutArgs;
+        using ::testing::Pointee;
         EXPECT_CALL(*mock, convertValue("a"));
         EXPECT_CALL(*mock, convertValue("b"))
             .WillOnce(DoAll(InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue),
                             InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue)));
-        EXPECT_CALL(*mock, processSet(3));
+        EXPECT_CALL(*mock, processSetValues(Pointee(ElementsAre("a", "dummy", "dummy"))));
         EXPECT_CALL(*mock, processAll());
     }
 
@@ -278,12 +272,14 @@ TEST(AbstractOptionStorageTest, HandlesTooManyValueAddition)
     {
         ::testing::InSequence dummy;
         using ::testing::DoAll;
+        using ::testing::ElementsAre;
         using ::testing::InvokeWithoutArgs;
+        using ::testing::Pointee;
         EXPECT_CALL(*mock, convertValue("a"));
         EXPECT_CALL(*mock, convertValue("b"))
             .WillOnce(DoAll(InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue),
                             InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue)));
-        EXPECT_CALL(*mock, processSet(2));
+        EXPECT_CALL(*mock, processSetValues(Pointee(ElementsAre("a", "dummy"))));
         EXPECT_CALL(*mock, processAll());
     }
 
@@ -311,17 +307,18 @@ TEST(AbstractOptionStorageTest, AllowsEmptyValues)
     std::vector<std::string>    values;
     MockOptionStorage          *mock;
     ASSERT_NO_THROW(options.addOption(
-                        MockOption("name").storageObject(&mock).mayNotAddValues()
+                        MockOption("name").storageObject(&mock)
                             .storeVector(&values).valueCount(0)));
 
     {
         ::testing::InSequence dummy;
-        using ::testing::_;
         using ::testing::DoAll;
+        using ::testing::ElementsAre;
+        using ::testing::Pointee;
         using ::testing::Return;
         EXPECT_CALL(*mock, convertValue("a"))
             .WillOnce(Return());
-        EXPECT_CALL(*mock, processSet(0));
+        EXPECT_CALL(*mock, processSetValues(Pointee(ElementsAre())));
         EXPECT_CALL(*mock, processAll());
     }
 
index d62171ac2701222c9f9a723ccf9fcfbc6afe0237..c5afbec62f7074b624b94cef13d20a106115d501 100644 (file)
@@ -80,6 +80,10 @@ TEST(OptionsAssignerTest, HandlesInvalidMultipleParameter)
     EXPECT_THROW(assigner.startOption("p"), gmx::InvalidInputError);
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options.finish());
+
+    EXPECT_TRUE(options.isSet("p"));
+    ASSERT_EQ(1U, values.size());
+    EXPECT_EQ(1, values[0]);
 }
 
 TEST(OptionsAssignerTest, HandlesMultipleParameter)
@@ -124,6 +128,8 @@ TEST(OptionsAssignerTest, HandlesMissingValue)
     EXPECT_NO_THROW(assigner.finishOption());
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(2, value2);
 }
 
 TEST(OptionsAssignerTest, HandlesExtraValue)
@@ -141,6 +147,8 @@ TEST(OptionsAssignerTest, HandlesExtraValue)
     EXPECT_NO_THROW(assigner.finishOption());
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(2, value1);
 }
 
 TEST(OptionsAssignerTest, HandlesSubSections)
@@ -507,6 +515,32 @@ TEST(OptionsAssignerIntegerTest, HandlesVectorsWithDefaultValue)
     EXPECT_EQ(1, vec[2]);
 }
 
+TEST(OptionsAssignerIntegerTest, HandlesVectorsWithDefaultValueWithInvalidAssignment)
+{
+    gmx::Options options(NULL, NULL);
+    int  vec[3] = {3, 2, 1};
+    std::vector<int> vec2(vec, vec+3);
+    using gmx::IntegerOption;
+    ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(vec)
+                                          .storeVector(&vec2).vector()));
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("p"));
+    EXPECT_NO_THROW(assigner.appendValue("1"));
+    EXPECT_NO_THROW(assigner.appendValue("3"));
+    EXPECT_THROW(assigner.finishOption(), gmx::InvalidInputError);
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(3, vec[0]);
+    EXPECT_EQ(2, vec[1]);
+    EXPECT_EQ(1, vec[2]);
+    ASSERT_EQ(3U, vec2.size());
+    EXPECT_EQ(3, vec2[0]);
+    EXPECT_EQ(2, vec2[1]);
+    EXPECT_EQ(1, vec2[2]);
+}
 
 TEST(OptionsAssignerDoubleTest, StoresSingleValue)
 {
index db3c5a012cd4894c9994ca29fbdcd7b81d5d97e0..3521b6ce1180436497c1c1bc4a6875d611bfd0ee 100644 (file)
@@ -61,8 +61,7 @@ namespace gmx
 SelectionOptionStorage::SelectionOptionStorage(const SelectionOption &settings,
                                                Options *options)
     : MyBase(settings, options,
-             OptionFlags() | efNoDefaultValue | efConversionMayNotAddValues
-                | efDontCheckMinimumCount),
+             OptionFlags() | efNoDefaultValue | efDontCheckMinimumCount),
       _selectionFlags(settings._selectionFlags), _adjuster(NULL)
 {
     options->globalProperties().request(eogpSelectionCollection);
@@ -95,6 +94,10 @@ void SelectionOptionStorage::addSelections(
     {
         GMX_THROW(InvalidInputError("Too few selections provided"));
     }
+    if (bFullValue)
+    {
+        clearSet();
+    }
     std::vector<Selection *>::const_iterator i;
     for (i = selections.begin(); i != selections.end(); ++i)
     {
@@ -107,6 +110,10 @@ void SelectionOptionStorage::addSelections(
         (*i)->setFlags(_selectionFlags);
         addValue(*i);
     }
+    if (bFullValue)
+    {
+        commitValues();
+    }
 }
 
 
@@ -122,14 +129,12 @@ void SelectionOptionStorage::convertValue(const std::string &value)
     addSelections(selections, false);
 }
 
-void SelectionOptionStorage::processSet(int nvalues)
+void SelectionOptionStorage::processSetValues(ValueList *values)
 {
-    if (nvalues > 0 && nvalues < minValueCount())
+    if (values->size() > 0 && values->size() < static_cast<size_t>(minValueCount()))
     {
-        // TODO: Remove the invalid values
         GMX_THROW(InvalidInputError("Too few (valid) values provided"));
     }
-    MyBase::processSet(nvalues);
 }
 
 void SelectionOptionStorage::processAll()
@@ -143,7 +148,6 @@ void SelectionOptionStorage::processAll()
         sc->_impl->requestSelections(name(), description(), this);
         setFlag(efSet);
     }
-    MyBase::processAll();
 }
 
 void SelectionOptionStorage::setAllowedValueCount(int count)
index b62b7945b7cb5f47a38900152bd40ca8fe7dd1a7..ed71f848431daf23591a6ef79d1c20e1330cf805 100644 (file)
@@ -111,7 +111,7 @@ class SelectionOptionStorage : public OptionStorageTemplate<Selection *>
 
     private:
         virtual void convertValue(const std::string &value);
-        virtual void processSet(int nvalues);
+        virtual void processSetValues(ValueList *values);
         virtual void processAll();
 
         SelectionFlags          _selectionFlags;