More consistent default handling for StringOption

For enumerated StringOptions, the enum indexes were not always
consistently handled.  Added a few tests for the behavior, and fixed it.
Now the storeEnumIndex() option works consistently in that if the caller
specifies an initial value for the variable, and there is no other
default, the caller-provided value is preserved.  Also, the stored enum
index is properly updated if a default value is provided through a value
in the output variable.

Fixes #1586

Change-Id: Ib9d24e977dbfde6b23ba90271574cdd43c70c1bd

diff --git a/src/gromacs/options/basicoptions.cpp b/src/gromacs/options/basicoptions.cpp
index 2b2c0863352ea777d349438ee6b4401c6e740182..8dff27360da2a41ccdeabc470018b69d93cf0a56 100644 (file)
--- a/src/gromacs/options/basicoptions.cpp
+++ b/src/gromacs/options/basicoptions.cpp
@@ -522,11 +522,6 @@ StringOptionStorage::StringOptionStorage(const StringOption &settings)
                 GMX_THROW(APIError("Conflicting default values"));
             }
         }
-        // If there is no default value, match is still -1.
-        if (enumIndexStore_ != NULL)
-        {
-            *enumIndexStore_ = match;
-        }
     }
     if (settings.defaultEnumIndex_ >= 0)
     {
@@ -534,6 +529,12 @@ StringOptionStorage::StringOptionStorage(const StringOption &settings)
         addValue(allowed_[settings.defaultEnumIndex_]);
         commitValues();
     }
+    // Somewhat subtly, this does not update the stored enum index if the
+    // caller has not provided store() or storeVector(), because values()
+    // will be empty in such a case.  This leads to (desired) behavior of
+    // preserving the existing value in the enum index store variable in such
+    // cases.
+    refreshEnumIndexStore();
 }
 
 std::string StringOptionStorage::formatExtraDescription() const
@@ -592,15 +593,27 @@ void StringOptionStorage::convertValue(const std::string &value)
 void StringOptionStorage::refreshValues()
 {
     MyBase::refreshValues();
+    refreshEnumIndexStore();
+}
+
+void StringOptionStorage::refreshEnumIndexStore()
+{
     if (enumIndexStore_ != NULL)
     {
         for (size_t i = 0; i < values().size(); ++i)
         {
-            ValueList::const_iterator match =
-                std::find(allowed_.begin(), allowed_.end(), values()[i]);
-            GMX_ASSERT(match != allowed_.end(),
-                       "Enum value not found (internal error)");
-            enumIndexStore_[i] = static_cast<int>(match - allowed_.begin());
+            if (values()[i].empty())
+            {
+                enumIndexStore_[i] = -1;
+            }
+            else
+            {
+                ValueList::const_iterator match =
+                    std::find(allowed_.begin(), allowed_.end(), values()[i]);
+                GMX_ASSERT(match != allowed_.end(),
+                           "Enum value not found (internal error)");
+                enumIndexStore_[i] = static_cast<int>(match - allowed_.begin());
+            }
         }
     }
 }

diff --git a/src/gromacs/options/basicoptions.h b/src/gromacs/options/basicoptions.h
index c35ee9228bef1768e6a2a8b4b314d8905f0d3276..67f14e31d9414018c57e06deef57e00da2c586fe 100644 (file)
--- a/src/gromacs/options/basicoptions.h
+++ b/src/gromacs/options/basicoptions.h
@@ -364,7 +364,10 @@ class StringOption : public OptionTemplate<std::string, StringOption>
          * The index (zero-based) of the selected value in the array \p values
          * provided to enumValues() is written into \p *store after the
          * option gets its value.  If the option has not been provided,
-         * and there is no default value, -1 is stored.
+         * and there is no default value, -1 is stored.  If store(),
+         * storeVector() or defaultEnumIndex() is not present, the value in
+         * \p *store is kept as a default value, otherwise it is always
+         * overwritten.
          *
          * Cannot be specified without enumValue().
          *

diff --git a/src/gromacs/options/basicoptionstorage.h b/src/gromacs/options/basicoptionstorage.h
index 02529cd7a224c8cd665d300beca7034cc8134da7..823ec97beb3e4ddca7ab0a5301c82c80c01a7b20 100644 (file)
--- a/src/gromacs/options/basicoptionstorage.h
+++ b/src/gromacs/options/basicoptionstorage.h
@@ -206,6 +206,8 @@ class StringOptionStorage : public OptionStorageTemplate<std::string>
         virtual void convertValue(const std::string &value);
         virtual void refreshValues();
 
+        void refreshEnumIndexStore();
+
         StringOptionInfo        info_;
         ValueList               allowed_;
         int                    *enumIndexStore_;

diff --git a/src/gromacs/options/tests/optionsassigner.cpp b/src/gromacs/options/tests/optionsassigner.cpp
index 91331d0768bd7d8661e3055089a31b412676edf1..9b6e244c0a5a4976b7b5124704a0fd6aeea4331c 100644 (file)
--- a/src/gromacs/options/tests/optionsassigner.cpp
+++ b/src/gromacs/options/tests/optionsassigner.cpp
@@ -826,6 +826,56 @@ TEST(OptionsAssignerStringTest, HandlesEnumDefaultValue)
     EXPECT_EQ(1, index);
 }
 
+TEST(OptionsAssignerStringTest, HandlesEnumDefaultValueFromVariable)
+{
+    gmx::Options           options(NULL, NULL);
+    std::string            value("test");
+    const char * const     allowed[] = { "none", "test", "value" };
+    int                    index     = -1;
+    using gmx::StringOption;
+    ASSERT_NO_THROW(options.addOption(
+                            StringOption("p").store(&value)
+                                .enumValue(allowed).storeEnumIndex(&index)));
+    EXPECT_EQ("test", value);
+    EXPECT_EQ(1, index);
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ("test", value);
+    EXPECT_EQ(1, index);
+}
+
+TEST(OptionsAssignerStringTest, HandlesEnumDefaultValueFromVector)
+{
+    gmx::Options             options(NULL, NULL);
+    std::vector<std::string> value;
+    value.push_back("test");
+    value.push_back("value");
+    const char * const       allowed[] = { "none", "test", "value" };
+    int                      index[2]  = {-1, -1};
+    using gmx::StringOption;
+    ASSERT_NO_THROW(options.addOption(
+                            StringOption("p").storeVector(&value).valueCount(2)
+                                .enumValue(allowed).storeEnumIndex(index)));
+    EXPECT_EQ("test", value[0]);
+    EXPECT_EQ("value", value[1]);
+    EXPECT_EQ(1, index[0]);
+    EXPECT_EQ(2, index[1]);
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ("test", value[0]);
+    EXPECT_EQ("value", value[1]);
+    EXPECT_EQ(1, index[0]);
+    EXPECT_EQ(2, index[1]);
+}
+
 TEST(OptionsAssignerStringTest, HandlesEnumDefaultIndex)
 {
     gmx::Options           options(NULL, NULL);
@@ -849,4 +899,23 @@ TEST(OptionsAssignerStringTest, HandlesEnumDefaultIndex)
     EXPECT_EQ(1, index);
 }
 
+TEST(OptionsAssignerStringTest, HandlesEnumDefaultIndexFromVariable)
+{
+    gmx::Options           options(NULL, NULL);
+    const char * const     allowed[] = { "none", "test", "value" };
+    int                    index     = 1;
+    using gmx::StringOption;
+    ASSERT_NO_THROW(options.addOption(
+                            StringOption("p")
+                                .enumValue(allowed).storeEnumIndex(&index)));
+    EXPECT_EQ(1, index);
+
+    gmx::OptionsAssigner assigner(&options);
+    EXPECT_NO_THROW(assigner.start());
+    EXPECT_NO_THROW(assigner.finish());
+    EXPECT_NO_THROW(options.finish());
+
+    EXPECT_EQ(1, index);
+}
+
 } // namespace