diff --git a/CHANGELOG.md b/CHANGELOG.md index 9eaa0e86d..84c66de9a 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/odr/internal/util/number_util.cpp b/src/odr/internal/util/number_util.cpp index d7d0f3935..e96980a01 100644 --- a/src/odr/internal/util/number_util.cpp +++ b/src/odr/internal/util/number_util.cpp @@ -26,20 +26,22 @@ std::optional 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(std::floor(std::log10(std::abs(value)))) + 1; + static_cast(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::clamp( + std::int64_t{significant_digits} - integer_digits, 0, 15)); std::string result = fmt::format("{:.{}f}", value, decimals); diff --git a/src/odr/internal/util/number_util.hpp b/src/odr/internal/util/number_util.hpp index dfd3b7c97..9bdf2495e 100644 --- a/src/odr/internal/util/number_util.hpp +++ b/src/odr/internal/util/number_util.hpp @@ -1,21 +1,20 @@ #pragma once +#include #include #include #include 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 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 diff --git a/src/odr/quantity.cpp b/src/odr/quantity.cpp index 6bf41b1d1..347e5dc23 100644 --- a/src/odr/quantity.cpp +++ b/src/odr/quantity.cpp @@ -1,6 +1,8 @@ #include +#include #include +#include #include #include @@ -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, 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) diff --git a/src/odr/quantity.hpp b/src/odr/quantity.hpp index ef9d168e5..b015bdd57 100644 --- a/src/odr/quantity.hpp +++ b/src/odr/quantity.hpp @@ -1,8 +1,9 @@ #pragma once -#include -#include -#include +#include +#include +#include +#include #include #include #include @@ -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(*end))) { - ++end; + std::string_view rest = string; + const double magnitude = parse_magnitude(rest); + if constexpr (std::is_integral_v) { + const double limit = + std::ldexp(1.0, std::numeric_limits::digits); + const double minimum = std::is_signed_v ? -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); + m_unit = Unit(rest); } bool operator==(const Quantity &rhs) const { @@ -76,9 +83,7 @@ class Quantity : private QuantityBase { return format_magnitude(static_cast(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(); } } diff --git a/test/src/internal/util/number_util_test.cpp b/test/src/internal/util/number_util_test.cpp index 820439d78..5ed38f712 100644 --- a/test/src/internal/util/number_util_test.cpp +++ b/test/src/internal/util/number_util_test.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include @@ -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::max()), + "0.5"); + EXPECT_EQ( + to_string_significant(1234, std::numeric_limits::min()), + "1234"); +} diff --git a/test/src/quantity_test.cpp b/test/src/quantity_test.cpp index 158668862..d856f70be 100644 --- a/test/src/quantity_test.cpp +++ b/test/src/quantity_test.cpp @@ -2,11 +2,19 @@ #include +#include +#include +#include + using namespace odr; TEST(Quantity, construct) { - Quantity{"10ms"}; - Quantity{"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 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(1234, DynamicUnit("cm")).to_string(), + "1234cm"); +} + +TEST(Quantity, checks_integral_magnitudes) { + EXPECT_EQ(Quantity("-42cm").magnitude(), -42); + EXPECT_THROW(Quantity("2147483648cm"), std::out_of_range); + EXPECT_THROW(Quantity("-1cm"), std::out_of_range); + EXPECT_THROW(Quantity("18446744073709551616cm"), + std::out_of_range); +}