improve options parsing (#1616)

First pass on the new options IO. Here we improve parsing for the existing `bool`, `int`, `double`, `ratio_t` types.

Also split the IO tests from the options structure tests (the options tests could possibly be reduced in the future as they currently also do some parsing/formatting test).

library/private/options_tools.h.in

@@ -4,6 +4,7 @@
 #include "options.h"
 #include "types.h"
 
+#include <algorithm>
 #include <sstream>
 
 namespace f3d
@@ -23,9 +24,36 @@ std::string trim(std::string_view strv)
   return std::string(strv);
 }
 
+//----------------------------------------------------------------------------
+/** Call `std::stod` and throw `std::invalid_argument` if the whole string has not been consumed. */
+double stodStrict(const std::string& str)
+{
+  std::size_t pos = 0;
+  const double parsed = std::stod(str, &pos);
+  if (pos != str.size())
+  {
+    throw std::invalid_argument("partial");
+  }
+  return parsed;
+}
+
+//----------------------------------------------------------------------------
+/** Call `std::stoi` and throw `std::invalid_argument` if the whole string has not been consumed */
+int stoiStrict(const std::string& str)
+{
+  std::size_t pos = 0;
+  const int parsed = std::stoi(str, &pos);
+  if (pos != str.size())
+  {
+    throw std::invalid_argument("partial");
+  }
+  return parsed;
+}
+
 //----------------------------------------------------------------------------
 /**
- * Vector specific templated parse method
+ * Vector specific templated parse method.
+ * Splits on `","` and trims chunks before parsing each element
  */
 template<typename T>
 struct is_vector : std::false_type
@@ -45,25 +73,29 @@ T parse(const std::string& str)
   std::istringstream split(str);
   for (std::string each; std::getline(split, each, ',');)
   {
-    vec.emplace_back(options_tools::parse<typename T::value_type>(each));
+    vec.emplace_back(options_tools::parse<typename T::value_type>(options_tools::trim(each)));
   }
   return vec;
 }
 
 //----------------------------------------------------------------------------
 /**
- * Parse provided string into a bool
- * Numerical and boolaplha are both supported
+ * Parse provided string into a bool. Supports boolapha values as well as "yes/no" and "on/off"
  */
 template<>
 bool parse(const std::string& str)
 {
-  // TODO implement proper parsing, eg "True", "TRUE", "ON", "On", ...
-  bool b1;
-  bool b2;
-  std::istringstream(str) >> b1;
-  std::istringstream(str) >> std::boolalpha >> b2;
-  return b1 || b2;
+  std::string s = str;
+  std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return std::tolower(c); });
+  if (s == "true" || s == "yes" || s == "on" || s == "1")
+  {
+    return true;
+  }
+  if (s == "false" || s == "no" || s == "off" || s == "0")
+  {
+    return false;
+  }
+  throw options::parsing_exception("Cannot parse " + str + " into a bool");
 }
 
 //----------------------------------------------------------------------------
@@ -75,10 +107,9 @@ bool parse(const std::string& str)
 template<>
 int parse(const std::string& str)
 {
-  // TODO implement more parsing possibilities ?
   try
   {
-    return std::stoi(str);
+    return stoiStrict(str);
   }
   catch (std::invalid_argument const&)
   {
@@ -100,10 +131,9 @@ int parse(const std::string& str)
 template<>
 double parse(const std::string& str)
 {
-  // TODO implement more parsing possibilities ?
   try
   {
-    return std::stod(str);
+    return stodStrict(str);
   }
   catch (std::invalid_argument const&)
   {
@@ -118,15 +148,37 @@ double parse(const std::string& str)
 
 //----------------------------------------------------------------------------
 /**
- * Parse provided string into a ratio_t
- * Rely on <double> version for parsing
+ * Parse provided string into a ratio_t.
+ * Supported formats: number (same as `<double>` version), percentage, fraction
  * Can throw options::parsing_exception in case of failure to parse
  */
 template<>
 ratio_t parse(const std::string& str)
 {
-  // TODO implement proper ratio parsing
-  return options_tools::parse<double>(str);
+  try
+  {
+    if (!str.empty() && str.at(str.size() - 1) == '%')
+    {
+      return stodStrict(str.substr(0, str.size() - 1)) / 100;
+    }
+
+    const std::size_t i = str.find('/');
+    if (i != std::string::npos)
+    {
+      return stodStrict(str.substr(0, i)) / stodStrict(str.substr(i + 1, str.size() - i - 1));
+    }
+
+    return stodStrict(str);
+  }
+  catch (std::invalid_argument const&)
+  {
+    throw options::parsing_exception("Cannot parse " + str + " into a ratio_t");
+  }
+  catch (std::out_of_range const&)
+  {
+    throw options::parsing_exception(
+      "Cannot parse " + str + " into a ratio_t as it would go out of range");
+  }
 }
 
 //----------------------------------------------------------------------------

library/public/options.h.in

@@ -108,8 +108,17 @@ public:
 
   /**
    * Templated parsing method used internally to parse strings.
-   * Implemented only for types: bool, int, double, ratio_t,
-   * as well as vector for each of these types.
+   * Implemented for:
+   * - `bool` as `"true"`/`"false"`, `"1"`/`"0"`, `"yes"`/`"no"`, `"on"`/`"off"`
+   * (case-insensitive)
+   * - `int`: `"123"`, `"+123"`, `"-456"`
+   * - `double`: `"123.45"`, `"+123.45"`, `"-67.8"`, `"-1e3"`
+   * - `ratio_t`: as
+   *   + a number, same a `double` above
+   *   + a percentage: `"12.3%"` => `0.123`
+   *   + a fraction: `"3/4"` => `0.75`
+   * - `vector<T>` where `T` is one of the above: the input is split on `","` and chunks are trimmed
+   *   before parsing each element.
    * See parsing documentation for more info. TODO.
    * Throw an options::parsing_exception if parsing failed.
    */

library/testing/CMakeLists.txt

@@ -18,6 +18,7 @@ list(APPEND libf3dSDKTests_list
      TestSDKMultiColoring.cxx
      TestSDKMultiOptions.cxx
      TestSDKOptions.cxx
+     TestSDKOptionsIO.cxx
      TestSDKRenderAndInteract.cxx
      TestSDKRenderFinalShader.cxx
      TestSDKWindowNative.cxx
@@ -87,6 +88,7 @@ endif()
 list(APPEND libf3dSDKTestsNoRender_list
      TestSDKEngineExceptions
      TestSDKOptions
+     TestSDKOptionsIO
      TestSDKLog
      TestSDKLoader)
 

library/testing/PseudoUnitTest.h

@@ -108,6 +108,28 @@ class PseudoUnitTest
     this->log(success, label, message);
   }
 
+  template<typename T>
+  std::string toString(const T& value)
+  {
+    std::stringstream ss;
+    ss << value;
+    return ss.str();
+  }
+
+  template<typename T>
+  std::string toString(const std::vector<T>& value)
+  {
+    std::stringstream ss;
+    size_t i = 0;
+    for (const T& item : value)
+    {
+      ss << (i++ ? ", " : "{ ") << this->toString(item);
+    }
+    ss << " }";
+    return ss.str();
+  }
+
+protected:
   template<typename T1, typename T2>
   std::string comparisonMessage(const T1& actual, const T2& expected, const std::string& comp)
   {
@@ -125,15 +147,6 @@ class PseudoUnitTest
     }
   }
 
-  template<typename T>
-  std::string toString(const T& value)
-  {
-    std::stringstream ss;
-    ss << value;
-    return ss.str();
-  }
-
-protected:
   virtual void log(const bool success, const std::string& label, const std::string& message)
   {
     const std::string line = message.empty() ? label : (label + ": " + message);

library/testing/TestSDKOptions.cxx

@@ -138,50 +138,5 @@ int TestSDKOptions(int argc, char* argv[])
   test.expect<f3d::options::inexistent_exception>(
     "inexistent_exception exception on copy", [&]() { opt.copy(opt2, "dummy"); });
 
-  // Test parse
-  if (f3d::options::parse<bool>(std::string("true")) != true)
-  {
-    std::cerr << "Options parse method not behaving as expected with bool." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<int>(std::string("37")) != 37)
-  {
-    std::cerr << "Options parse method not behaving as expected with int." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<double>(std::string("37.2")) != 37.2)
-  {
-    std::cerr << "Options parse method not behaving as expected with double." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<f3d::ratio_t>(std::string("0.31")) != 0.31)
-  {
-    std::cerr << "Options parse method not behaving as expected with ratio_t." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<std::string>(std::string("  foobar  ")) != "foobar")
-  {
-    std::cerr << "Options parse method not behaving as expected with string." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<std::vector<int>>(std::string("1, 2, 3")) !=
-    std::vector<int>({ 1, 2, 3 }))
-  {
-    std::cerr << "Options parse method not behaving as expected with int vector." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<std::vector<double>>(std::string("0.1, 0.2, 0.3")) !=
-    std::vector<double>({ 0.1, 0.2, 0.3 }))
-  {
-    std::cerr << "Options parse method not behaving as expected with double vector." << std::endl;
-    return EXIT_FAILURE;
-  }
-  if (f3d::options::parse<std::vector<std::string>>(std::string("foo, bar, baz")) !=
-    std::vector<std::string>({ "foo", "bar", "baz" }))
-  {
-    std::cerr << "Options parse method not behaving as expected with string vector." << std::endl;
-    return EXIT_FAILURE;
-  }
-
   return EXIT_SUCCESS;
 }

library/testing/TestSDKOptionsIO.cxx

@@ -0,0 +1,75 @@
+#include <export.h>
+#include <options.h>
+
+#include "PseudoUnitTest.h"
+
+#include <iostream>
+
+class ParsingTest : public PseudoUnitTest
+{
+public:
+  template<typename T>
+  void parse(const std::string& label, const std::string& input, const T& expected)
+  {
+    PseudoUnitTest::operator()(label + " `" + input + "`", [&]() {
+      const T actual = f3d::options::parse<T>(input);
+      if (actual != expected)
+      {
+        throw this->comparisonMessage(actual, expected, "!=");
+      }
+    });
+  }
+
+  template<typename T, typename E>
+  void parse_expect(const std::string& label, const std::string& input)
+  {
+    PseudoUnitTest::expect<E>(label + " `" + input + "`", [&]() { f3d::options::parse<T>(input); });
+  }
+};
+
+int TestSDKOptionsIO(int argc, char* argv[])
+{
+  using parsing_exception = f3d::options::parsing_exception;
+  ParsingTest test;
+
+  test.parse<bool>("bool", "true", true);
+  test.parse<bool>("bool", "false", false);
+  test.parse<bool>("bool", "yes", true);
+  test.parse<bool>("bool", "Yes", true);
+  test.parse<bool>("bool", "no", false);
+  test.parse<bool>("bool", "1", true);
+  test.parse<bool>("bool", "0", false);
+  test.parse_expect<bool, parsing_exception>("invalid bool", "foo");
+
+  test.parse<int>("int", "123", 123);
+  test.parse<int>("int", "-123", -123);
+  test.parse<int>("int", "+123", +123);
+  test.parse_expect<int, parsing_exception>("invalid int", "1.2");
+  test.parse_expect<int, parsing_exception>("invalid int", "abc");
+  test.parse_expect<int, parsing_exception>("invalid int",
+    "123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890");
+
+  test.parse<double>("double", "123", 123.0);
+  test.parse<double>("double", "-123.45", -123.45);
+  test.parse<double>("double", "+1e-3", 0.001);
+  test.parse_expect<double, parsing_exception>("invalid double", "1.2.3");
+  test.parse_expect<double, parsing_exception>("invalid double", "abc");
+
+  test.parse<std::string>("std::string", "foobar", "foobar");
+  test.parse<std::string>("std::string", "  foobar   ", "foobar");
+
+  test.parse<f3d::ratio_t>("ratio_t", "0.1234", 0.1234);
+  test.parse<f3d::ratio_t>("ratio_t", "12.34%", 0.1234);
+  test.parse_expect<f3d::ratio_t, parsing_exception>("invalid ratio_t", "12.34&");
+  test.parse<f3d::ratio_t>("ratio_t", "-2/-3.5", 2.0 / 3.5);
+  test.parse_expect<f3d::ratio_t, parsing_exception>("invalid ratio_t", "1/2/3");
+
+  test.parse<std::vector<int>>("std::vector<int>", "1, 2, 3", { 1, 2, 3 });
+
+  test.parse<std::vector<double>>("std::vector<double>", "  0.1,  0.2 , 0.3 ", { 0.1, 0.2, 0.3 });
+
+  test.parse<std::vector<std::string>>(
+    "std::vector<std::string>", "  foo, bar ,   baz ", { "foo", "bar", "baz" });
+
+  return test.result();
+}