From: Teemu Murtola Date: Tue, 26 Aug 2014 16:28:43 +0000 (+0300) Subject: More consistent default handling for StringOption X-Git-Url: http://biod.pnpi.spb.ru/gitweb/?p=alexxy%2Fgromacs.git;a=commitdiff_plain;h=236deaf1539a55c3f03ff40e16175ffcb65a6cfe 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 2b2c086335..8dff27360d 100644 --- 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(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(match - allowed_.begin()); + } } } } diff --git a/src/gromacs/options/basicoptions.h b/src/gromacs/options/basicoptions.h index c35ee9228b..67f14e31d9 100644 --- a/src/gromacs/options/basicoptions.h +++ b/src/gromacs/options/basicoptions.h @@ -364,7 +364,10 @@ class StringOption : public OptionTemplate * 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 02529cd7a2..823ec97beb 100644 --- a/src/gromacs/options/basicoptionstorage.h +++ b/src/gromacs/options/basicoptionstorage.h @@ -206,6 +206,8 @@ class StringOptionStorage : public OptionStorageTemplate 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 91331d0768..9b6e244c0a 100644 --- 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 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