Proper support for multiple values @ ConfigDefinition

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";
       }

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<T>
     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<T> defaultValue;
-    nonstd::optional<T> parsedValue; // needs to be set when parseValue() called
+    std::vector<T> parsedValues;
     std::function<void(T)> 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

test/config/test_llarp_config_definition.cpp

@@ -10,13 +10,13 @@ TEST_CASE("OptionDefinition int parse test", "[config]")
   llarp::OptionDefinition<int> 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<int> values;
+  config.defineOption<int>("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);
+}