Improve integer parsing

Fixes #39. Closes #40. This is an overhaul of the way that integer arguments are parsed. Instead of using std::istream, which allows, for example, negative integers for unsigned types, we use our own parser. This allows us to do proper range checking depending on the type, and to correctly check for negative values passed to unsigned types. This also allows the handling of base 16 numbers.

Improve integer parsing
Fixes #39. Closes #40. This is an overhaul of the way that integer arguments are parsed. Instead of using std::istream, which allows, for example, negative integers for unsigned types, we use our own parser. This allows us to do proper range checking depending on the type, and to correctly check for negative values passed to unsigned types. This also allows the handling of base 16 numbers.
Jarryd Beck
1 parent f931fd42
Showing 2 changed files with 254 additions and 6 deletions
include/cxxopts.hpp
test/options.cpp
 /*
-Copyright (c) 2014, 2015, 2016 Jarryd Beck
+Copyright (c) 2014, 2015, 2016, 2017 Jarryd Beck
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
@@ -31,6 +31,7 @@ THE SOFTWARE.
 #endif
 #include <cstring>
+#include <cctype>
 #include <exception>
 #include <iostream>
 #include <map>
@@ -411,20 +412,170 @@ namespace cxxopts
   namespace values
   {
+    std::basic_regex<char> integer_pattern
+      ("(-)?(0x)?([1-9a-zA-Z][0-9a-zA-Z]*)|(0)");
+
+    namespace detail
+    {
+      template <typename T, bool B>
+      struct SignedCheck;
+
+      template <typename T>
+      struct SignedCheck<T, true>
+      {
+        template <typename U>
+        void
+        operator()(bool negative, U u, const std::string& text)
+        {
+          if (negative)
+          {
+            if (u > U(-std::numeric_limits<T>::min()))
+            {
+              throw argument_incorrect_type(text);
+            }
+          }
+          else
+          {
+            if (u > std::numeric_limits<T>::max())
+            {
+              throw argument_incorrect_type(text);
+            }
+          }
+        }
+      };
+
+      template <typename T>
+      struct SignedCheck<T, false>
+      {
+        template <typename U>
+        void
+        operator()(bool, U, const std::string&) {}
+      };
+
+      template <typename T, typename U>
+      void
+      check_signed_range(bool negative, U value, const std::string& text)
+      {
+        SignedCheck<T, std::numeric_limits<T>::is_signed>()(negative, value, text);
+      }
+    }
+
     template <typename T>
     void
-    parse_value(const std::string& text, T& value)
+    integer_parser(const std::string& text, T& value)
     {
-      std::istringstream is(text);
-      if (!(is >> value))
+      std::smatch match;
+      std::regex_match(text, match, integer_pattern);
+
+      if (match.length() == 0)
       {
         throw argument_incorrect_type(text);
       }
-      if (is.rdbuf()->in_avail() != 0)
+      if (match.length(4) > 0)
       {
-        throw argument_incorrect_type(text);
+        value = 0;
+        return;
+      }
+
+      using US = typename std::make_unsigned<T>::type;
+
+      constexpr auto umax = std::numeric_limits<US>::max();
+      constexpr bool is_signed = std::numeric_limits<T>::is_signed;
+      const bool negative = match.length(1) > 0;
+      const auto base = match.length(2) > 0 ? 16 : 10;
+
+      auto value_match = match[3];
+
+      US result = 0;
+
+      for (auto iter = value_match.first; iter != value_match.second; ++iter)
+      {
+        int digit = 0;
+
+        if (*iter >= '0' && *iter <= '9')
+        {
+          digit = *iter - '0';
+        }
+        else if (*iter >= 'a' && *iter <= 'f')
+        {
+          digit = *iter - 'a' + 10;
+        }
+        else if (*iter >= 'A' && *iter <= 'F')
+        {
+          digit = *iter - 'A' + 10;
+        }
+
+        if (umax - digit < result * base)
+        {
+          throw argument_incorrect_type(text);
+        }
+
+        result = result * base + digit;
       }
+
+      detail::check_signed_range<T>(negative, result, text);
+
+      if (negative)
+      {
+        if (!is_signed)
+        {
+          throw argument_incorrect_type(text);
+        }
+        value = -result;
+      }
+      else
+      {
+        value = result;
+      }
+    }
+
+    void
+    parse_value(const std::string& text, uint8_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, int8_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, uint16_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, int16_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, uint32_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, int32_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, uint64_t& value)
+    {
+      integer_parser(text, value);
+    }
+
+    void
+    parse_value(const std::string& text, int64_t& value)
+    {
+      integer_parser(text, value);
     }
     inline
@@ -239,3 +239,100 @@ TEST_CASE(&quot;Empty with implicit value&quot;, &quot;[implicit]&quot;)
   REQUIRE(options.count("implicit") == 1);
   REQUIRE(options["implicit"].as<std::string>() == "");
 }
+
+TEST_CASE("Integers", "[options]")
+{
+  cxxopts::Options options("parses_integers", "parses integers correctly");
+  options.add_options()
+    ("positional", "Integers", cxxopts::value<std::vector<int>>());
+
+  Argv av({"ints", "--", "5", "6", "-6", "0", "0xab", "0xAf"});
+
+  char** argv = av.argv();
+  auto argc = av.argc();
+
+  options.parse_positional("positional");
+  options.parse(argc, argv);
+
+  REQUIRE(options.count("positional") == 6);
+
+  auto& positional = options["positional"].as<std::vector<int>>();
+  CHECK(positional[0] == 5);
+  CHECK(positional[1] == 6);
+  CHECK(positional[2] == -6);
+  CHECK(positional[3] == 0);
+  CHECK(positional[4] == 0xab);
+  CHECK(positional[5] == 0xaf);
+}
+
+TEST_CASE("Unsigned integers", "[options]")
+{
+  cxxopts::Options options("parses_unsigned", "detects unsigned errors");
+  options.add_options()
+    ("positional", "Integers", cxxopts::value<std::vector<unsigned int>>());
+
+  Argv av({"ints", "--", "-2"});
+
+  char** argv = av.argv();
+  auto argc = av.argc();
+
+  options.parse_positional("positional");
+  CHECK_THROWS_AS(options.parse(argc, argv), cxxopts::argument_incorrect_type);
+}
+
+TEST_CASE("Integer bounds", "[integer]")
+{
+  cxxopts::Options options("integer_boundaries", "check min/max integer");
+  options.add_options()
+    ("positional", "Integers", cxxopts::value<std::vector<int8_t>>());
+
+  SECTION("No overflow")
+  {
+    Argv av({"ints", "--", "127", "-128", "0x7f", "-0x80", "0x7e"});
+
+    auto argv = av.argv();
+    auto argc = av.argc();
+
+    options.parse_positional("positional");
+    options.parse(argc, argv);
+
+    REQUIRE(options.count("positional") == 5);
+
+    auto& positional = options["positional"].as<std::vector<int8_t>>();
+    CHECK(positional[0] == 127);
+    CHECK(positional[1] == -128);
+    CHECK(positional[2] == 0x7f);
+    CHECK(positional[3] == -0x80);
+    CHECK(positional[4] == 0x7e);
+  }
+}
+
+TEST_CASE("Overflow on boundary", "[integer]")
+{
+  using namespace cxxopts::values;
+
+  int8_t si;
+  uint8_t ui;
+
+  CHECK_THROWS_AS((integer_parser("128", si)), cxxopts::argument_incorrect_type);
+  CHECK_THROWS_AS((integer_parser("-129", si)), cxxopts::argument_incorrect_type);
+  CHECK_THROWS_AS((integer_parser("256", ui)), cxxopts::argument_incorrect_type);
+  CHECK_THROWS_AS((integer_parser("-0x81", si)), cxxopts::argument_incorrect_type);
+  CHECK_THROWS_AS((integer_parser("0x80", si)), cxxopts::argument_incorrect_type);
+  CHECK_THROWS_AS((integer_parser("0x100", ui)), cxxopts::argument_incorrect_type);
+}
+
+TEST_CASE("Integer overflow", "[options]")
+{
+  cxxopts::Options options("reject_overflow", "rejects overflowing integers");
+  options.add_options()
+    ("positional", "Integers", cxxopts::value<std::vector<int8_t>>());
+
+  Argv av({"ints", "--", "128"});
+
+  auto argv = av.argv();
+  auto argc = av.argc();
+
+  options.parse_positional("positional");
+  CHECK_THROWS_AS(options.parse(argc, argv), cxxopts::argument_incorrect_type);
+}