From cb46397dfb4bb939d45ffe86aeafb8972fd157d3 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Sat, 27 Apr 2019 07:13:35 -0700 Subject: [PATCH 1/4] Fix typo --- test/format-test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/format-test.cc b/test/format-test.cc index e1547cb1..94ebcdd5 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -2451,7 +2451,7 @@ TEST(FormatTest, FormatStringErrors) { EXPECT_ERROR("{:d}", "invalid type specifier", std::string); EXPECT_ERROR("{:s}", "invalid type specifier", void*); # else - fmt::print("warning: constexpr is broken in this versio of MSVC\n"); + fmt::print("warning: constexpr is broken in this version of MSVC\n"); # endif EXPECT_ERROR("{foo", "compile-time checks don't support named arguments", int); From 40a79756407301a1beb87c9ecd1c2040bd9347a2 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Sat, 27 Apr 2019 07:42:27 -0700 Subject: [PATCH 2/4] Remove trailing zeros --- include/fmt/format.h | 2 ++ test/format-test.cc | 1 + 2 files changed, 3 insertions(+) diff --git a/include/fmt/format.h b/include/fmt/format.h index 0e8fb2e4..e426f3bc 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -1213,6 +1213,8 @@ It grisu2_prettify(const char* digits, int size, int exp, It it, if (params.num_digits >= 0 && params.num_digits < num_zeros) num_zeros = params.num_digits; it = std::fill_n(it, num_zeros, static_cast('0')); + if (!params.trailing_zeros) + while (size > 0 && digits[size - 1] == '0') --size; it = copy_str(digits, digits + size, it); } return it; diff --git a/test/format-test.cc b/test/format-test.cc index 94ebcdd5..6cd1b136 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -1488,6 +1488,7 @@ TEST(FormatterTest, PrecisionRounding) { EXPECT_EQ("0.002", format("{:.3f}", 0.0015)); EXPECT_EQ("1.000", format("{:.3f}", 0.9999)); EXPECT_EQ("0.00123", format("{:.3}", 0.00123)); + EXPECT_EQ("0.1", format("{:.16g}", 0.1)); // Trigger rounding error in Grisu by a carefully chosen number. auto n = 3788512123356.985352; char buffer[64]; From 8d8ea21c6947b0b5c579878965add8e7b2e8e7cc Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Sat, 27 Apr 2019 06:52:46 -0700 Subject: [PATCH 3/4] Partially implement Grisu3 --- include/fmt/format-inl.h | 79 +++++++++++++++++++++++++++++----------- include/fmt/format.h | 29 +++++++++------ src/format.cc | 6 +-- test/format-impl-test.cc | 4 +- 4 files changed, 81 insertions(+), 37 deletions(-) diff --git a/include/fmt/format-inl.h b/include/fmt/format-inl.h index a074f6ae..30df00df 100644 --- a/include/fmt/format-inl.h +++ b/include/fmt/format-inl.h @@ -494,10 +494,12 @@ enum result { }; } -// Generates output using Grisu2 digit-gen algorithm. +// Generates output using the Grisu digit-gen algorithm. +// error: the size of the region (lower, upper) outside of which numbers +// definitely do not round to value (Delta in Grisu3). template -digits::result grisu2_gen_digits(fp value, uint64_t error, int& exp, - Handler& handler) { +digits::result grisu_gen_digits(fp value, uint64_t error, int& exp, + Handler& handler) { fp one(1ull << -value.e, value.e); // The integral part of scaled value (p1 in Grisu) = value / one. It cannot be // zero because it contains a product of two 64-bit numbers with MSB set (due @@ -635,34 +637,57 @@ struct fixed_handler { }; // The shortest representation digit handler. -struct shortest_handler { +template struct grisu_shortest_handler { char* buf; int size; - fp diff; // wp_w in Grisu. + // Distance between scaled value and upper bound (wp_W in Grisu3). + uint64_t diff; digits::result on_start(uint64_t, uint64_t, uint64_t, int&) { return digits::more; } + + // This implements Grisu3's round_weed. digits::result on_digit(char digit, uint64_t divisor, uint64_t remainder, uint64_t error, int exp, bool integral) { buf[size++] = digit; - if (remainder > error) return digits::more; - uint64_t d = integral ? diff.f : diff.f * data::POWERS_OF_10_64[-exp]; - while ( - remainder < d && error - remainder >= divisor && - (remainder + divisor < d || d - remainder > remainder + divisor - d)) { + if (remainder >= error) return digits::more; + if (GRISU_VERSION != 3) { + uint64_t d = integral ? diff : diff * data::POWERS_OF_10_64[-exp]; + while (remainder < d && error - remainder >= divisor && + (remainder + divisor < d || + d - remainder > remainder + divisor - d)) { + --buf[size - 1]; + remainder += divisor; + } + return digits::done; + } + uint64_t unit = integral ? 1 : data::POWERS_OF_10_64[-exp]; + uint64_t up = diff + unit; // wp_Wup + while (remainder < up && error - remainder >= divisor && + (remainder + divisor < up || + up - remainder > remainder + divisor - up)) { --buf[size - 1]; remainder += divisor; } - return digits::done; + uint64_t down = diff - unit; // wp_Wdown + if (remainder < down && error - remainder >= divisor && + (remainder + divisor < down || + down - remainder > remainder + divisor - down)) { + return digits::error; + } + return 2 * unit <= remainder && remainder <= error - 4 * unit + ? digits::done + : digits::error; } }; template ::type> -FMT_FUNC bool grisu2_format(Double value, buffer& buf, int precision, - bool fixed, int& exp) { +FMT_FUNC bool grisu_format(Double value, buffer& buf, int precision, + unsigned options, int& exp) { FMT_ASSERT(value >= 0, "value is negative"); + bool fixed = (options & grisu_options::fixed) != 0; if (value <= 0) { // <= instead of == to silence a warning. if (precision < 0 || !fixed) { exp = 0; @@ -685,7 +710,7 @@ FMT_FUNC bool grisu2_format(Double value, buffer& buf, int precision, min_exp - (fp_value.e + fp::significand_size), cached_exp10); fp_value = fp_value * cached_pow; fixed_handler handler{buf.data(), 0, precision, -cached_exp10, fixed}; - if (grisu2_gen_digits(fp_value, 1, exp, handler) == digits::error) + if (grisu_gen_digits(fp_value, 1, exp, handler) == digits::error) return false; buf.resize(to_unsigned(handler.size)); } else { @@ -695,17 +720,29 @@ FMT_FUNC bool grisu2_format(Double value, buffer& buf, int precision, // the exponent in the range [min_exp, -32]. auto cached_pow = get_cached_power( // \tilde{c}_{-k} in Grisu. min_exp - (upper.e + fp::significand_size), cached_exp10); - upper = upper * cached_pow; // \tilde{M}^+ in Grisu. - --upper.f; // \tilde{M}^+ - 1 ulp -> M^+_{\downarrow}. - assert(min_exp <= upper.e && upper.e <= -32); fp_value.normalize(); fp_value = fp_value * cached_pow; lower = lower * cached_pow; // \tilde{M}^- in Grisu. - ++lower.f; // \tilde{M}^- + 1 ulp -> M^-_{\uparrow}. - shortest_handler handler{buf.data(), 0, upper - fp_value}; - auto result = grisu2_gen_digits(upper, upper.f - lower.f, exp, handler); + upper = upper * cached_pow; // \tilde{M}^+ in Grisu. + assert(min_exp <= upper.e && upper.e <= -32); + auto result = digits::result(); + int size = 0; + if ((options & grisu_options::grisu3) != 0) { + --lower.f; // \tilde{M}^- - 1 ulp -> M^-_{\downarrow}. + ++upper.f; // \tilde{M}^+ + 1 ulp -> M^+_{\uparrow}. + // Numbers outside of (lower, upper) definitely do not round to value. + grisu_shortest_handler<3> handler{buf.data(), 0, (upper - fp_value).f}; + result = grisu_gen_digits(upper, upper.f - lower.f, exp, handler); + size = handler.size; + } else { + ++lower.f; // \tilde{M}^- + 1 ulp -> M^-_{\uparrow}. + --upper.f; // \tilde{M}^+ - 1 ulp -> M^+_{\downarrow}. + grisu_shortest_handler<2> handler{buf.data(), 0, (upper - fp_value).f}; + result = grisu_gen_digits(upper, upper.f - lower.f, exp, handler); + size = handler.size; + } if (result == digits::error) return false; - buf.resize(to_unsigned(handler.size)); + buf.resize(to_unsigned(size)); } exp -= cached_exp10; return true; diff --git a/include/fmt/format.h b/include/fmt/format.h index e426f3bc..8e101c45 100644 --- a/include/fmt/format.h +++ b/include/fmt/format.h @@ -1124,13 +1124,19 @@ FMT_CONSTEXPR unsigned basic_parse_context::next_arg_id() { namespace internal { -// Formats value using Grisu2 algorithm: +namespace grisu_options { +enum { + fixed = 1, + grisu3 = 2 +}; +} + +// Formats value using the Grisu algorithm: // https://www.cs.tufts.edu/~nr/cs257/archive/florian-loitsch/printf.pdf template -FMT_API bool grisu2_format(Double value, buffer& buf, int precision, - bool fixed, int& exp); +FMT_API bool grisu_format(Double, buffer&, int, unsigned, int&); template -inline bool grisu2_format(Double, buffer&, int, bool, int&) { +inline bool grisu_format(Double, buffer&, int, unsigned, int&) { return false; } @@ -1166,8 +1172,8 @@ template It write_exponent(int exp, It it) { // The number is given as v = digits * pow(10, exp). template -It grisu2_prettify(const char* digits, int size, int exp, It it, - gen_digits_params params) { +It grisu_prettify(const char* digits, int size, int exp, It it, + gen_digits_params params) { // pow(10, full_exp - 1) <= v <= pow(10, full_exp). int full_exp = size + exp; if (!params.fixed) { @@ -2663,7 +2669,7 @@ template class basic_writer { int full_exp = num_digits + exp - 1; int precision = params.num_digits > 0 ? params.num_digits : 11; params_.fixed |= full_exp >= -4 && full_exp < precision; - auto it = internal::grisu2_prettify( + auto it = internal::grisu_prettify( digits.data(), num_digits, exp, internal::counting_iterator(), params_); size_ = it.count(); @@ -2675,8 +2681,8 @@ template class basic_writer { template void operator()(It&& it) { if (sign_) *it++ = static_cast(sign_); int num_digits = static_cast(digits_.size()); - it = internal::grisu2_prettify(digits_.data(), num_digits, - exp_, it, params_); + it = internal::grisu_prettify(digits_.data(), num_digits, exp_, + it, params_); } }; @@ -2876,11 +2882,12 @@ void basic_writer::write_double(T value, const format_specs& spec) { memory_buffer buffer; int exp = 0; int precision = spec.has_precision() || !spec.type ? spec.precision : 6; + unsigned options = handler.fixed ? internal::grisu_options::fixed : 0; bool use_grisu = fmt::internal::use_grisu() && (spec.type != 'a' && spec.type != 'A' && spec.type != 'e' && spec.type != 'E') && - internal::grisu2_format(static_cast(value), buffer, - precision, handler.fixed, exp); + internal::grisu_format(static_cast(value), buffer, + precision, options, exp); if (!use_grisu) internal::sprintf_format(value, buffer, spec); if (handler.as_percentage) { diff --git a/src/format.cc b/src/format.cc index 7236def4..6b268bbe 100644 --- a/src/format.cc +++ b/src/format.cc @@ -10,9 +10,9 @@ FMT_BEGIN_NAMESPACE template struct internal::basic_data; -// Workaround a bug in MSVC2013 that prevents instantiation of grisu2_format. -bool (*instantiate_grisu2_format)(double, internal::buffer&, int, bool, - int&) = internal::grisu2_format; +// Workaround a bug in MSVC2013 that prevents instantiation of grisu_format. +bool (*instantiate_grisu_format)(double, internal::buffer&, int, unsigned, + int&) = internal::grisu_format; #ifndef FMT_STATIC_THOUSANDS_SEPARATOR template FMT_API internal::locale_ref::locale_ref(const std::locale& loc); diff --git a/test/format-impl-test.cc b/test/format-impl-test.cc index 6f48aee3..33263aaa 100644 --- a/test/format-impl-test.cc +++ b/test/format-impl-test.cc @@ -137,10 +137,10 @@ TEST(FPTest, FixedHandler) { digits::error); } -TEST(FPTest, Grisu2FormatCompilesWithNonIEEEDouble) { +TEST(FPTest, GrisuFormatCompilesWithNonIEEEDouble) { fmt::memory_buffer buf; int exp = 0; - grisu2_format(4.2f, buf, -1, false, exp); + grisu_format(4.2f, buf, -1, false, exp); } template struct ValueExtractor : fmt::internal::function { From 4c721e3a2f8cb0241b0e4a4781cf3fbd32da00a6 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Sun, 28 Apr 2019 07:08:41 -0700 Subject: [PATCH 4/4] Fix chrono formatting with invalid argument id (#1132) --- include/fmt/chrono.h | 31 +++++++++++++++++++++---------- test/chrono-test.cc | 5 +++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index b65bdfad..53563860 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -585,18 +585,20 @@ struct formatter, Char> { } }; - public: - formatter() : spec(), precision(-1) {} + typedef typename basic_parse_context::iterator iterator; + struct parse_range { + iterator begin; + iterator end; + }; - FMT_CONSTEXPR auto parse(basic_parse_context& ctx) - -> decltype(ctx.begin()) { + FMT_CONSTEXPR parse_range do_parse(basic_parse_context& ctx) { auto begin = ctx.begin(), end = ctx.end(); - if (begin == end) return begin; + if (begin == end) return {begin, end}; spec_handler handler{*this, ctx, format_str}; begin = internal::parse_align(begin, end, handler); - if (begin == end) return begin; + if (begin == end) return {begin, end}; begin = internal::parse_width(begin, end, handler); - if (begin == end) return begin; + if (begin == end) return {begin, end}; if (*begin == '.') { if (std::is_floating_point::value) begin = internal::parse_precision(begin, end, handler); @@ -604,9 +606,18 @@ struct formatter, Char> { handler.on_error("precision not allowed for this argument type"); } end = parse_chrono_format(begin, end, internal::chrono_format_checker()); - format_str = - basic_string_view(&*begin, internal::to_unsigned(end - begin)); - return end; + return {begin, end}; + } + + public: + formatter() : spec(), precision(-1) {} + + FMT_CONSTEXPR auto parse(basic_parse_context& ctx) + -> decltype(ctx.begin()) { + auto range = do_parse(ctx); + format_str = basic_string_view( + &*range.begin, internal::to_unsigned(range.end - range.begin)); + return range.end; } template diff --git a/test/chrono-test.cc b/test/chrono-test.cc index 40d34235..bb546f7b 100644 --- a/test/chrono-test.cc +++ b/test/chrono-test.cc @@ -296,4 +296,9 @@ TEST(ChronoTest, FormatFullSpecsQq) { EXPECT_EQ("*1.2340 ms*", fmt::format("{:*^11.4%Q %q}", dms(1.234))); } +TEST(ChronoTest, InvalidWidthId) { + EXPECT_THROW(fmt::format("{:{o}", std::chrono::seconds(0)), + fmt::format_error); +} + #endif // FMT_STATIC_THOUSANDS_SEPARATOR