Better invalid option value handling.

- Give an error when trying to give an empty string as a value for a
  numeric option, as well as when the given value overflows/underflows
  the target variable.
- Don't give extra errors (e.g., about too few valid values) when a
  value for an option was invalid.
- Make assignment more atomic: if there was an error in a set of values,
  ignore other values.

Change-Id: I30fd471f6d65246b4215a5ee775f0d16492a21d0

diff --git a/src/gromacs/options/abstractoption.cpp b/src/gromacs/options/abstractoption.cpp
index 7522211f47cc52a962eab317418ff86f64a4306a..a4d8685bcfee2f2625ffd91fd72a042be64f1893 100644 (file)
--- a/src/gromacs/options/abstractoption.cpp
+++ b/src/gromacs/options/abstractoption.cpp
@@ -57,7 +57,7 @@ AbstractOptionStorage::AbstractOptionStorage(const AbstractOption &settings,
     : flags_(settings.flags_ | staticFlags),
       minValueCount_(settings.minValueCount_),
       maxValueCount_(settings.maxValueCount_),
-      inSet_(false)
+      bInSet_(false), bSetValuesHadErrors_(false)
 {
     // Check that user has not provided incorrect values for vectors.
     if (hasFlag(efOption_Vector) && (minValueCount_ > 1 || maxValueCount_ < 1))
@@ -89,7 +89,7 @@ void AbstractOptionStorage::startSource()
 
 void AbstractOptionStorage::startSet()
 {
-    GMX_RELEASE_ASSERT(!inSet_, "finishSet() not called");
+    GMX_RELEASE_ASSERT(!bInSet_, "finishSet() not called");
     // 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.
@@ -99,32 +99,48 @@ void AbstractOptionStorage::startSet()
         GMX_THROW(InvalidInputError("Option specified multiple times"));
     }
     clearSet();
-    inSet_ = true;
+    bInSet_ = true;
+    bSetValuesHadErrors_ = false;
 }
 
 void AbstractOptionStorage::appendValue(const std::string &value)
 {
-    GMX_RELEASE_ASSERT(inSet_, "startSet() not called");
-    convertValue(value);
+    GMX_RELEASE_ASSERT(bInSet_, "startSet() not called");
+    try
+    {
+        convertValue(value);
+    }
+    catch (...)
+    {
+        bSetValuesHadErrors_ = true;
+        throw;
+    }
 }
 
 void AbstractOptionStorage::finishSet()
 {
-    GMX_RELEASE_ASSERT(inSet_, "startSet() not called");
-    inSet_ = false;
-    // TODO: Should this be set or not when processSet() throws?
+    GMX_RELEASE_ASSERT(bInSet_, "startSet() not called");
+    bInSet_ = false;
+    // We mark the option as set even when there are errors to avoid additional
+    // errors from required options not set.
+    // TODO: There could be a separate flag for this purpose.
     setFlag(efOption_Set);
-    // TODO: Correct handling of the efOption_ClearOnNextSet requires
-    // processSet() and/or convertValue() to check it internally.
-    // OptionStorageTemplate takes care of it, but it's error-prone if
-    // a custom option is implemented that doesn't use it.
-    processSet();
+    if (!bSetValuesHadErrors_)
+    {
+        // TODO: Correct handling of the efOption_ClearOnNextSet requires
+        // processSet() and/or convertValue() to check it internally.
+        // OptionStorageTemplate takes care of it, but it's error-prone if
+        // a custom option is implemented that doesn't use it.
+        processSet();
+    }
+    bSetValuesHadErrors_ = false;
     clearFlag(efOption_ClearOnNextSet);
+    clearSet();
 }
 
 void AbstractOptionStorage::finish()
 {
-    GMX_RELEASE_ASSERT(!inSet_, "finishSet() not called");
+    GMX_RELEASE_ASSERT(!bInSet_, "finishSet() not called");
     processAll();
     if (isRequired() && !isSet())
     {

diff --git a/src/gromacs/options/abstractoptionstorage.h b/src/gromacs/options/abstractoptionstorage.h
index 6f8a7c023b5aefb46ea6aaeb46d1801612e6dda5..5c1f297d0bac618f0c2a74bab157d100284977c1 100644 (file)
--- a/src/gromacs/options/abstractoptionstorage.h
+++ b/src/gromacs/options/abstractoptionstorage.h
@@ -277,6 +277,11 @@ class AbstractOptionStorage
          *
          * This method may be called multiple times if the underlying option
          * can be specified multiple times.
+         * This method is not currently called if one of the convertValue()
+         * calls throwed.
+         *
+         * \todo
+         * Improve the call semantics.
          *
          * \see OptionStorageTemplate::processSetValues()
          */
@@ -306,7 +311,9 @@ class AbstractOptionStorage
         //! Maximum allowed number of values (in one set), or -1 if no limit.
         int                     maxValueCount_;
         //! Whether we are currently assigning values to a set.
-        bool                    inSet_;
+        bool                    bInSet_;
+        //! Whether there were errors in set values.
+        bool                    bSetValuesHadErrors_;
 
         GMX_DISALLOW_COPY_AND_ASSIGN(AbstractOptionStorage);
 };

diff --git a/src/gromacs/options/basicoptions.cpp b/src/gromacs/options/basicoptions.cpp
index 7715fe7caab888e6f3dd2c7399a530f846c64345..96ec04d8592d9bb18378945bf02f209bed90f5e8 100644 (file)
--- a/src/gromacs/options/basicoptions.cpp
+++ b/src/gromacs/options/basicoptions.cpp
@@ -38,9 +38,11 @@
  */
 #include "gromacs/options/basicoptions.h"
 
+#include <cerrno>
 #include <cstdio>
 #include <cstdlib>
 
+#include <limits>
 #include <string>
 #include <vector>
 
@@ -143,10 +145,19 @@ void IntegerOptionStorage::convertValue(const std::string &value)
 {
     const char *ptr = value.c_str();
     char *endptr;
+    errno = 0;
     long int ival = std::strtol(ptr, &endptr, 10);
-    if (*endptr != '\0')
+    if (errno == ERANGE
+        || ival < std::numeric_limits<int>::min()
+        || ival > std::numeric_limits<int>::max())
     {
-        GMX_THROW(InvalidInputError("Invalid value: " + value));
+        GMX_THROW(InvalidInputError("Invalid value: '" + value
+                                    + "'; it causes an integer overflow"));
+    }
+    if (*ptr == '\0' || *endptr != '\0')
+    {
+        GMX_THROW(InvalidInputError("Invalid value: '" + value
+                                    + "'; expected an integer"));
     }
     addValue(ival);
 }
@@ -201,10 +212,17 @@ void DoubleOptionStorage::convertValue(const std::string &value)
 {
     const char *ptr = value.c_str();
     char *endptr;
+    errno = 0;
     double dval = std::strtod(ptr, &endptr);
-    if (*endptr != '\0')
+    if (errno == ERANGE)
+    {
+        GMX_THROW(InvalidInputError("Invalid value: '" + value
+                                    + "'; it causes an overflow/underflow"));
+    }
+    if (*ptr == '\0' || *endptr != '\0')
     {
-        GMX_THROW(InvalidInputError("Invalid value: " + value));
+        GMX_THROW(InvalidInputError("Invalid value: '" + value
+                                    + "'; expected a number"));
     }
     addValue(dval * factor_);
 }

diff --git a/src/gromacs/options/tests/abstractoptionstorage.cpp b/src/gromacs/options/tests/abstractoptionstorage.cpp
index d1b0595c9e118d158922588a94e24cb4f5e6b0ac..f6b7c998648429ef75b0bd30dfe6be1b62be2b5f 100644 (file)
--- a/src/gromacs/options/tests/abstractoptionstorage.cpp
+++ b/src/gromacs/options/tests/abstractoptionstorage.cpp
@@ -294,7 +294,6 @@ TEST(AbstractOptionStorageTest, HandlesTooManyValueAddition)
         EXPECT_CALL(*mock, convertValue("b"))
             .WillOnce(DoAll(InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue),
                             InvokeWithoutArgs(mock, &MockOptionStorage::addDummyValue)));
-        EXPECT_CALL(*mock, processSetValues(Pointee(ElementsAre("a", "dummy"))));
         EXPECT_CALL(*mock, processAll());
     }
 
@@ -307,9 +306,7 @@ TEST(AbstractOptionStorageTest, HandlesTooManyValueAddition)
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options.finish());
 
-    ASSERT_EQ(2U, values.size());
-    EXPECT_EQ("a", values[0]);
-    EXPECT_EQ("dummy", values[1]);
+    ASSERT_TRUE(values.empty());
 }
 
 /*

diff --git a/src/gromacs/options/tests/optionsassigner.cpp b/src/gromacs/options/tests/optionsassigner.cpp
index 6ae3ac4ebca700f7a25d070ffd58bc223a40e600..60f927481a8bf23cfb0e00f8f72cb26c565d67af 100644 (file)
--- a/src/gromacs/options/tests/optionsassigner.cpp
+++ b/src/gromacs/options/tests/optionsassigner.cpp
@@ -41,6 +41,7 @@
  * \author Teemu Murtola <teemu.murtola@cbr.su.se>
  * \ingroup module_options
  */
+#include <limits>
 #include <vector>
 
 #include <gtest/gtest.h>
@@ -49,10 +50,15 @@
 #include "gromacs/options/options.h"
 #include "gromacs/options/optionsassigner.h"
 #include "gromacs/utility/exceptions.h"
+#include "gromacs/utility/stringutil.h"
 
 namespace
 {
 
+/********************************************************************
+ * General assignment tests
+ */
+
 TEST(OptionsAssignerTest, HandlesMissingRequiredParameter)
 {
     gmx::Options options(NULL, NULL);
@@ -148,7 +154,7 @@ TEST(OptionsAssignerTest, HandlesExtraValue)
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options.finish());
 
-    EXPECT_EQ(2, value1);
+    EXPECT_EQ(0, value1);
 }
 
 TEST(OptionsAssignerTest, HandlesSubSections)
@@ -266,6 +272,10 @@ TEST(OptionsAssignerTest, HandlesMultipleSources)
 }
 
 
+/********************************************************************
+ * Tests for boolean assignment
+ */
+
 TEST(OptionsAssignerBooleanTest, StoresYesValue)
 {
     gmx::Options options(NULL, NULL);
@@ -344,6 +354,12 @@ TEST(OptionsAssignerBooleanTest, HandlesBooleanWithPrefixAndValue)
 }
 
 
+/********************************************************************
+ * Tests for integer assignment
+ *
+ * These tests also contain tests for general default value handling.
+ */
+
 TEST(OptionsAssignerIntegerTest, StoresSingleValue)
 {
     gmx::Options options(NULL, NULL);
@@ -362,6 +378,62 @@ TEST(OptionsAssignerIntegerTest, StoresSingleValue)
     EXPECT_EQ(3, value);
 }
 
+TEST(OptionsAssignerIntegerTest, HandlesEmptyValue)
+{
+    gmx::Options options(NULL, NULL);
+    int value = 1;
+    using gmx::IntegerOption;
+    ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&value)));
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("p"));
+    EXPECT_THROW(assigner.appendValue(""), gmx::InvalidInputError);
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(1, value);
+}
+
+TEST(OptionsAssignerIntegerTest, HandlesInvalidValue)
+{
+    gmx::Options options(NULL, NULL);
+    int value = 1;
+    using gmx::IntegerOption;
+    ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&value)));
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("p"));
+    EXPECT_THROW(assigner.appendValue("2abc"), gmx::InvalidInputError);
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(1, value);
+}
+
+TEST(OptionsAssignerIntegerTest, HandlesOverflow)
+{
+    gmx::Options options(NULL, NULL);
+    int value = 1;
+    using gmx::IntegerOption;
+    ASSERT_NO_THROW(options.addOption(IntegerOption("p").store(&value)));
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("p"));
+    std::string overflowValue(
+            gmx::formatString("%d0000", std::numeric_limits<int>::max()));
+    EXPECT_THROW(assigner.appendValue(overflowValue), gmx::InvalidInputError);
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(1, value);
+}
+
 TEST(OptionsAssignerIntegerTest, StoresDefaultValue)
 {
     gmx::Options options(NULL, NULL);
@@ -542,6 +614,11 @@ TEST(OptionsAssignerIntegerTest, HandlesVectorsWithDefaultValueWithInvalidAssign
     EXPECT_EQ(1, vec2[2]);
 }
 
+
+/********************************************************************
+ * Tests for double assignment
+ */
+
 TEST(OptionsAssignerDoubleTest, StoresSingleValue)
 {
     gmx::Options options(NULL, NULL);
@@ -560,6 +637,28 @@ TEST(OptionsAssignerDoubleTest, StoresSingleValue)
     EXPECT_DOUBLE_EQ(2.7, value);
 }
 
+TEST(OptionsAssignerDoubleTest, HandlesEmptyValue)
+{
+    gmx::Options options(NULL, NULL);
+    double value = 1.0;
+    using gmx::DoubleOption;
+    ASSERT_NO_THROW(options.addOption(DoubleOption("p").store(&value)));
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    ASSERT_NO_THROW(assigner.startOption("p"));
+    EXPECT_THROW(assigner.appendValue(""), gmx::InvalidInputError);
+    EXPECT_NO_THROW(assigner.finishOption());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_DOUBLE_EQ(1.0, value);
+}
+
+
+/********************************************************************
+ * Tests for string assignment
+ */
 
 TEST(OptionsAssignerStringTest, StoresSingleValue)
 {

diff --git a/src/gromacs/selection/tests/selectionoption.cpp b/src/gromacs/selection/tests/selectionoption.cpp
index 5a3bad5dfab506990f1f04c98b1e3f3dcdcf3ae3..2f228025f367f5b04c7021eb29fdb797757e31a9 100644 (file)
--- a/src/gromacs/selection/tests/selectionoption.cpp
+++ b/src/gromacs/selection/tests/selectionoption.cpp
@@ -139,7 +139,6 @@ TEST_F(SelectionOptionTest, HandlesTooManySelections)
     EXPECT_NO_THROW(assigner.finishOption());
     EXPECT_NO_THROW(assigner.finish());
     EXPECT_NO_THROW(options_.finish());
-    ASSERT_STREQ("resname RA RB", sel.selectionText());
 }