From 0a9515a94ae242ebebbe4f4f63fa1bd4273a43ef Mon Sep 17 00:00:00 2001 From: Stephen Shelton Date: Wed, 1 Apr 2020 11:10:12 -0600 Subject: [PATCH] Proper support for multiple values @ ConfigDefinition --- llarp/config/definition.cpp | 6 +- llarp/config/definition.hpp | 80 +++++++++++++++----- test/config/test_llarp_config_definition.cpp | 37 +++++++-- 3 files changed, 93 insertions(+), 30 deletions(-) diff --git a/llarp/config/definition.cpp b/llarp/config/definition.cpp index 09616186c..13833062e 100644 --- a/llarp/config/definition.cpp +++ b/llarp/config/definition.cpp @@ -98,14 +98,14 @@ ConfigDefinition::validateRequiredFields() { visitDefinitions(section, [&](const std::string&, const OptionDefinition_ptr& def) { - if (def->required and def->numFound < 1) + if (def->required and def->getNumberFound() < 1) { throw std::invalid_argument(stringify( "[", section, "]:", def->name, " is required but missing")); } // should be handled earlier in OptionDefinition::parseValue() - assert(def->numFound <= 1 or def->multiValued); + assert(def->getNumberFound() <= 1 or def->multiValued); }); }); } @@ -176,7 +176,7 @@ ConfigDefinition::generateINIConfig(bool useValues) oss << "# " << comment << "\n"; } - if (useValues and def->numFound > 0) + if (useValues and def->getNumberFound() > 0) { oss << name << "=" << def->valueAsString(false) << "\n"; } diff --git a/llarp/config/definition.hpp b/llarp/config/definition.hpp index 1a8dae218..95676ac48 100644 --- a/llarp/config/definition.hpp +++ b/llarp/config/definition.hpp @@ -42,6 +42,12 @@ namespace llarp virtual void parseValue(const std::string& input) = 0; + /// Subclasses should provide the number of values found. + /// + /// @return number of values found + virtual size_t + getNumberFound() const = 0; + /// Subclasess should write their parsed value as a string, optionally falling back to any /// specified default if `useDefault` is true. /// @@ -59,7 +65,6 @@ namespace llarp std::string name; bool required = false; bool multiValued = false; - size_t numFound = 0; }; /// The primary type-aware implementation of OptionDefinitionBase, this templated class allows @@ -105,21 +110,45 @@ namespace llarp { } - /// Returns the parsed value, if available. Otherwise, provides the default value if the option - /// is not required. Otherwise, returns an empty optional. + /// Returns the first parsed value, if available. Otherwise, provides the default value if the + /// option is not required. Otherwise, returns an empty optional. /// /// @return an optional with the parsed value, the default value, or no value. nonstd::optional getValue() const { - if (parsedValue) - return parsedValue.value(); - else if (not required) + if (parsedValues.size()) + return parsedValues[0]; + else if (not required and not multiValued) return defaultValue.value(); else return {}; } + /// Returns the value at the given index. + /// + /// @param index + /// @return the value at the given index, if it exists + /// @throws range_error exception if index >= size + T + getValueAt(size_t index) const + { + if (index >= parsedValues.size()) + throw std::range_error(stringify( + "no value at index ", index, ", size: ", parsedValues.size())); + + return parsedValues[index]; + } + + /// Returns the number of values found. + /// + /// @return number of values found + size_t + getNumberFound() const override + { + return parsedValues.size(); + } + std::string defaultValueAsString() override { @@ -133,10 +162,10 @@ namespace llarp void parseValue(const std::string& input) override { - if (not multiValued and parsedValue.has_value()) + if (not multiValued and parsedValues.size() > 0) { throw std::invalid_argument(stringify("duplicate value for ", name, - ", previous value: ", parsedValue.value())); + ", previous value: ", parsedValues[0])); } std::istringstream iss(input); @@ -148,8 +177,7 @@ namespace llarp } else { - parsedValue = t; - numFound++; + parsedValues.emplace_back(std::move(t)); } } @@ -158,8 +186,8 @@ namespace llarp valueAsString(bool useDefault) override { std::ostringstream oss; - if (parsedValue.has_value()) - oss << parsedValue.value(); + if (parsedValues.size() > 0) + oss << parsedValues[0]; else if (useDefault and defaultValue.has_value()) oss << defaultValue.value(); @@ -174,23 +202,37 @@ namespace llarp void tryAccept() const override { - if (required and not parsedValue.has_value()) + if (required and parsedValues.size() == 0) { throw std::runtime_error(stringify("cannot call tryAccept() on [", section, "]:", name, " when required but no value available")); } + // don't use default value if we are multi-valued and have no value + if (multiValued && parsedValues.size() == 0) + return; + if (acceptor) { - auto maybe = getValue(); - assert(maybe.has_value()); // should be guaranteed by our earlier check - // TODO: avoid copies here if possible - acceptor(maybe.value()); + if (multiValued) + { + for (const auto& value : parsedValues) + { + acceptor(value); + } + } + else + { + auto maybe = getValue(); + assert(maybe.has_value()); // should be guaranteed by our earlier checks + // TODO: avoid copies here if possible + acceptor(maybe.value()); + } } } nonstd::optional defaultValue; - nonstd::optional parsedValue; // needs to be set when parseValue() called + std::vector parsedValues; std::function acceptor; }; @@ -248,7 +290,7 @@ namespace llarp /// called). /// /// If the specified option doesn't exist, an exception will be thrown. Otherwise, the option's - /// parsedValue() will be invoked, and should throw an exception if the string can't be parsed. + /// parseValue() will be invoked, and should throw an exception if the string can't be parsed. /// /// @param section is the section this value resides in /// @param name is the name of the value diff --git a/test/config/test_llarp_config_definition.cpp b/test/config/test_llarp_config_definition.cpp index 07db7804b..5aaae45c3 100644 --- a/test/config/test_llarp_config_definition.cpp +++ b/test/config/test_llarp_config_definition.cpp @@ -10,13 +10,13 @@ TEST_CASE("OptionDefinition int parse test", "[config]") llarp::OptionDefinition def("foo", "bar", false, 42); CHECK(def.getValue() == 42); - CHECK(def.numFound == 0); + CHECK(def.getNumberFound() == 0); CHECK(def.defaultValueAsString() == "42"); CHECK_NOTHROW(def.parseValue("43")); CHECK(def.getValue() == 43); - CHECK(def.numFound == 1); + CHECK(def.getNumberFound() == 1); CHECK(def.defaultValueAsString() == "42"); } @@ -30,7 +30,7 @@ TEST_CASE("OptionDefinition string parse test", "[config]") CHECK_NOTHROW(def.parseValue("foo")); CHECK(def.getValue() == "foo"); - CHECK(def.numFound == 1); + CHECK(def.getNumberFound() == 1); } TEST_CASE("OptionDefinition multiple parses test", "[config]") @@ -41,12 +41,13 @@ TEST_CASE("OptionDefinition multiple parses test", "[config]") CHECK_NOTHROW(def.parseValue("9")); CHECK(def.getValue() == 9); - CHECK(def.numFound == 1); + CHECK(def.getNumberFound() == 1); // should allow since it is multi-value CHECK_NOTHROW(def.parseValue("12")); - CHECK(def.getValue() == 12); - CHECK(def.numFound == 2); + CHECK(def.getValue() == 9); // getValue() should return first value + REQUIRE(def.getNumberFound() == 2); + } { @@ -54,11 +55,11 @@ TEST_CASE("OptionDefinition multiple parses test", "[config]") CHECK_NOTHROW(def.parseValue("3")); CHECK(def.getValue() == 3); - CHECK(def.numFound == 1); + CHECK(def.getNumberFound() == 1); // shouldn't allow since not multi-value CHECK_THROWS(def.parseValue("2")); - CHECK(def.numFound == 1); + CHECK(def.getNumberFound() == 1); } } @@ -342,3 +343,23 @@ TEST_CASE("ConfigDefinition AssignmentAcceptor", "[config]") REQUIRE(val == 3); } + +TEST_CASE("ConfigDefinition multiple values", "[config]") +{ + llarp::ConfigDefinition config; + + std::vector values; + config.defineOption("foo", "bar", false, true, 2, [&](int arg) { + values.push_back(arg); + }); + + config.addConfigValue("foo", "bar", "1"); + config.addConfigValue("foo", "bar", "2"); + config.addConfigValue("foo", "bar", "3"); + CHECK_NOTHROW(config.acceptAllOptions()); + + REQUIRE(values.size() == 3); + CHECK(values[0] == 1); + CHECK(values[1] == 2); + CHECK(values[2] == 3); +}