fix: parse quantities independently of the host locale

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Quantity parsing and integer formatting are locale independent. Invalid or
+  out-of-range integer magnitudes throw instead of silently converting.
+
 - Block-cipher helpers reject incomplete blocks before calling Crypto++, and
   password derivation rejects parameters that would be narrowed or truncated.
 

src/odr/internal/util/number_util.cpp

@@ -26,20 +26,22 @@ std::optional<double> number::parse(const std::string_view text) {
   return (stream >> std::ws).eof() ? std::optional(value) : std::nullopt;
 }
 
-std::string number::to_string_significant(const double value,
-                                          const int significant_digits) {
+std::string
+number::to_string_significant(const double value,
+                              const std::int32_t significant_digits) {
   if (!std::isfinite(value)) {
     return fmt::format("{}", value);
   }
 
   // `{:.Nf}` counts decimals, not significant digits, so shift by the integer
   // part; clamped because a denormal or a huge value would blow up
-  int integer_digits = 1;
+  std::int32_t integer_digits = 1;
   if (value != 0.0) {
     integer_digits =
-        static_cast<int>(std::floor(std::log10(std::abs(value)))) + 1;
+        static_cast<std::int32_t>(std::floor(std::log10(std::abs(value)))) + 1;
   }
-  const int decimals = std::clamp(significant_digits - integer_digits, 0, 15);
+  const auto decimals = static_cast<std::int32_t>(std::clamp<std::int64_t>(
+      std::int64_t{significant_digits} - integer_digits, 0, 15));
 
   std::string result = fmt::format("{:.{}f}", value, decimals);
 

src/odr/internal/util/number_util.hpp

@@ -1,21 +1,20 @@
 #pragma once
 
+#include <cstdint>
 #include <optional>
 #include <string>
 #include <string_view>
 
 namespace odr::internal::util::number {
 
-/// Reads @p text as a decimal number with a `.` separator, whatever the host's
-/// locale — a german one would read `1234.5` as `1234`. Only blanks may
+/// Reads a decimal with a `.` separator in any host locale. Only whitespace may
 /// surround it: a unit or a group separator is refused, not truncated.
 [[nodiscard]] std::optional<double> parse(std::string_view text);
 
-/// Renders @p value with @p significant_digits significant digits, without
-/// trailing zeros, never in scientific notation, which CSS and SVG lengths do
-/// not accept, and never in the host's locale, where a german one would write
-/// `1,5`. Asking for more digits than the source has shows its noise: a
-/// `float` carries about 7, beyond that `68.55` becomes `68.550003`.
-std::string to_string_significant(double value, int significant_digits);
+/// Formats significant digits without exponent notation, which CSS and SVG
+/// lengths do not accept, and without trailing zeros. More digits than the
+/// source carries show its noise: a `float` has about 7.
+std::string to_string_significant(double value,
+                                  std::int32_t significant_digits);
 
 } // namespace odr::internal::util::number

src/odr/quantity.cpp

@@ -1,6 +1,8 @@
 #include <odr/quantity.hpp>
 
+#include <odr/internal/common/text_cursor.hpp>
 #include <odr/internal/util/number_util.hpp>
+#include <odr/internal/util/string_util.hpp>
 
 #include <functional>
 #include <memory>
@@ -10,9 +12,41 @@
 
 namespace odr {
 
-/// 7 significant digits: one more than the stream default, which rounds
-/// drawing coordinates in the thousands of mm, and no more than a `float`
-/// carries, so `68.55` does not come back as `68.550003`.
+double QuantityBase::parse_magnitude(std::string_view &text) {
+  internal::TextCursor cursor(text);
+  cursor.skip_whitespace();
+  const std::string_view start = cursor.rest();
+  if (cursor.peek() == '+' || cursor.peek() == '-') {
+    cursor.advance(1);
+  }
+  const auto digit = internal::util::string::is_ascii_digit;
+  (void)cursor.take_while(digit);
+  if (cursor.peek() == '.') {
+    cursor.advance(1);
+    (void)cursor.take_while(digit);
+  }
+  if (cursor.peek() == 'e' || cursor.peek() == 'E') {
+    const std::string_view exponent = cursor.rest();
+    cursor.advance(1);
+    if (cursor.peek() == '+' || cursor.peek() == '-') {
+      cursor.advance(1);
+    }
+    if (cursor.take_while(digit).empty()) {
+      cursor.seek(exponent); // An `em` or `ex` unit is not an exponent.
+    }
+  }
+  const auto magnitude = internal::util::number::parse(
+      start.substr(0, start.size() - cursor.rest().size()));
+  if (!magnitude) {
+    throw std::invalid_argument("invalid quantity magnitude");
+  }
+  cursor.skip_whitespace();
+  text = cursor.rest();
+  return *magnitude;
+}
+
+/// 7 digits: enough for drawing coordinates in the thousands of mm, and no more
+/// than a `float` carries, so `68.55` does not come back as `68.550003`.
 std::string QuantityBase::format_magnitude(const double magnitude) {
   return internal::util::number::to_string_significant(magnitude, 7);
 }
@@ -48,10 +82,7 @@ class DynamicUnit::Registry final {
   std::unordered_map<std::string, std::unique_ptr<Unit>, Hash, std::equal_to<>>
       m_registry;
 
-  /// `std::unordered_map` keeps element addresses stable across a rehash, so a
-  /// `Unit *` already handed out survives later insertions and only the map
-  /// access itself needs guarding. Every `Measure` goes through here and the
-  /// http server renders on a thread pool, hence the shared read path.
+  /// Unit addresses remain stable; concurrent lookups share the read lock.
   const Unit *unit_(const std::string_view name) {
     {
       const std::shared_lock lock(m_mutex);
@@ -70,8 +101,7 @@ class DynamicUnit::Registry final {
   }
 };
 
-/// Registered rather than left null, so that `m_unit` is always dereferenceable
-/// and this compares equal to the unit `Measure("5")` parses.
+/// Registered, not null, so that it equals the unit `Measure("5")` parses.
 DynamicUnit::DynamicUnit() : m_unit{Registry::unit("")} {}
 
 DynamicUnit::DynamicUnit(const std::string_view name)

src/odr/quantity.hpp

@@ -1,8 +1,9 @@
 #pragma once
 
-#include <cctype>
-#include <cstdlib>
-#include <sstream>
+#include <cmath>
+#include <limits>
+#include <ostream>
+#include <stdexcept>
 #include <string>
 #include <string_view>
 #include <type_traits>
@@ -35,6 +36,8 @@ class DynamicUnit {
 /// once in a source file instead of inline in every instantiation.
 class QuantityBase {
 protected:
+  static double parse_magnitude(std::string_view &text);
+
   /// Renders @p magnitude with 7 significant digits, always positional.
   static std::string format_magnitude(double magnitude);
 };
@@ -47,14 +50,18 @@ class Quantity : private QuantityBase {
       : m_magnitude{std::move(magnitude)}, m_unit{std::move(unit)} {}
 
   explicit Quantity(const std::string_view string) {
-    // `std::strtod` needs a terminator, which a view does not promise
-    const std::string buffer(string);
-    char *end{nullptr};
-    m_magnitude = std::strtod(buffer.c_str(), &end);
-    while (*end != '\0' && std::isspace(static_cast<unsigned char>(*end))) {
-      ++end;
+    std::string_view rest = string;
+    const double magnitude = parse_magnitude(rest);
+    if constexpr (std::is_integral_v<Magnitude>) {
+      const double limit =
+          std::ldexp(1.0, std::numeric_limits<Magnitude>::digits);
+      const double minimum = std::is_signed_v<Magnitude> ? -limit : 0;
+      if (magnitude < minimum || magnitude >= limit) {
+        throw std::out_of_range("quantity magnitude out of range");
+      }
     }
-    m_unit = DynamicUnit(end);
+    m_magnitude = static_cast<Magnitude>(magnitude);
+    m_unit = Unit(rest);
   }
 
   bool operator==(const Quantity &rhs) const {
@@ -76,9 +83,7 @@ class Quantity : private QuantityBase {
       return format_magnitude(static_cast<double>(m_magnitude)) +
              m_unit.to_string();
     } else {
-      std::ostringstream ss;
-      ss << m_magnitude << m_unit.to_string();
-      return ss.str();
+      return std::to_string(m_magnitude) + m_unit.to_string();
     }
   }
 

test/src/internal/util/number_util_test.cpp

@@ -1,6 +1,7 @@
 #include <odr/internal/util/number_util.hpp>
 
 #include <cmath>
+#include <limits>
 #include <optional>
 
 #include <gtest/gtest.h>
@@ -63,3 +64,12 @@ TEST(ToStringSignificant, passes_through_non_finite) {
   EXPECT_EQ(to_string_significant(std::nan(""), 7), "nan");
   EXPECT_EQ(to_string_significant(HUGE_VAL, 7), "inf");
 }
+
+TEST(ToStringSignificant, bounds_extreme_precision) {
+  EXPECT_EQ(
+      to_string_significant(0.5, std::numeric_limits<std::int32_t>::max()),
+      "0.5");
+  EXPECT_EQ(
+      to_string_significant(1234, std::numeric_limits<std::int32_t>::min()),
+      "1234");
+}

test/src/quantity_test.cpp

@@ -2,11 +2,19 @@
 
 #include <gtest/gtest.h>
 
+#include <clocale>
+#include <cstdint>
+#include <locale>
+
 using namespace odr;
 
 TEST(Quantity, construct) {
-  Quantity<double>{"10ms"};
-  Quantity<double>{"10 ms"};
+  EXPECT_EQ(Measure("10ms"), Measure(10, DynamicUnit("ms")));
+  EXPECT_EQ(Measure("10 ms"), Measure(10, DynamicUnit("ms")));
+  EXPECT_EQ(Measure("1.5em"), Measure(1.5, DynamicUnit("em")));
+  EXPECT_EQ(Measure(" -2.5e-3 ex"), Measure(-0.0025, DynamicUnit("ex")));
+  EXPECT_THROW(Measure("cm"), std::invalid_argument);
+  EXPECT_THROW(Measure("1e999cm"), std::invalid_argument);
 }
 
 /// A default-constructed unit used to leave the unit pointer null, so rendering
@@ -28,3 +36,45 @@ TEST(Quantity, default_unit_equals_parsed_unitless) {
   EXPECT_EQ(Measure(0, DynamicUnit()), Measure("0"));
   EXPECT_EQ(Measure(2.5, {}), Measure("2.5"));
 }
+
+namespace {
+
+class GroupedNumbers final : public std::numpunct<char> {
+  char do_decimal_point() const override { return ','; }
+  char do_thousands_sep() const override { return '.'; }
+  std::string do_grouping() const override { return "\3"; }
+};
+
+class LocaleGuard final {
+public:
+  LocaleGuard() : m_cpp(), m_c(std::setlocale(LC_NUMERIC, nullptr)) {}
+  ~LocaleGuard() {
+    std::locale::global(m_cpp);
+    std::setlocale(LC_NUMERIC, m_c.c_str());
+  }
+
+private:
+  std::locale m_cpp;
+  std::string m_c;
+};
+
+} // namespace
+
+TEST(Quantity, ignores_numeric_locale) {
+  const LocaleGuard guard;
+  std::locale::global(std::locale(std::locale::classic(), new GroupedNumbers));
+  if (std::setlocale(LC_NUMERIC, "de_DE.UTF-8") == nullptr) {
+    std::setlocale(LC_NUMERIC, "de_DE.utf8");
+  }
+  EXPECT_EQ(Measure("1234.5cm"), Measure(1234.5, DynamicUnit("cm")));
+  EXPECT_EQ(Quantity<std::int32_t>(1234, DynamicUnit("cm")).to_string(),
+            "1234cm");
+}
+
+TEST(Quantity, checks_integral_magnitudes) {
+  EXPECT_EQ(Quantity<std::int32_t>("-42cm").magnitude(), -42);
+  EXPECT_THROW(Quantity<std::int32_t>("2147483648cm"), std::out_of_range);
+  EXPECT_THROW(Quantity<std::uint32_t>("-1cm"), std::out_of_range);
+  EXPECT_THROW(Quantity<std::uint64_t>("18446744073709551616cm"),
+               std::out_of_range);
+}