From 894faf3fedae80bd48759292f7ce3cb83db23193 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Mon, 6 Sep 2021 10:24:21 -0700 Subject: [PATCH] Refactor presentation types --- CMakeLists.txt | 2 +- include/fmt/chrono.h | 3 +- include/fmt/core.h | 154 +++++++++++++++++++++++++++----------- include/fmt/format.h | 43 ++++++----- include/fmt/printf.h | 19 +++-- test/core-test.cc | 17 +++-- test/format-test.cc | 4 +- test/fuzzing/named-arg.cc | 3 +- test/xchar-test.cc | 2 +- 9 files changed, 164 insertions(+), 83 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index fcaa7ffa..2d43f6f8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -151,7 +151,7 @@ if (CMAKE_CXX_COMPILER_ID MATCHES "GNU") -Wcast-align -Wctor-dtor-privacy -Wdisabled-optimization -Winvalid-pch -Woverloaded-virtual - -Wconversion -Wswitch-enum -Wundef + -Wconversion -Wundef -Wno-ctor-dtor-privacy -Wno-format-nonliteral) if (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.6) set(PEDANTIC_COMPILE_FLAGS ${PEDANTIC_COMPILE_FLAGS} diff --git a/include/fmt/chrono.h b/include/fmt/chrono.h index f3bf422d..0d591d83 100644 --- a/include/fmt/chrono.h +++ b/include/fmt/chrono.h @@ -904,7 +904,8 @@ template (); specs.precision = precision; - specs.type = precision > 0 ? 'f' : 'g'; + specs.type = precision > 0 ? presentation_type::fixed_lower + : presentation_type::general_lower; return write(out, val, specs); } diff --git a/include/fmt/core.h b/include/fmt/core.h index 198691aa..f69f8220 100644 --- a/include/fmt/core.h +++ b/include/fmt/core.h @@ -1949,11 +1949,33 @@ template struct fill_t { }; FMT_END_DETAIL_NAMESPACE +enum class presentation_type : unsigned char { + none, + // Integer types should go first, + dec, // 'd' + oct, // 'o' + hex_lower, // 'x' + hex_upper, // 'X' + bin_lower, // 'b' + bin_upper, // 'B' + hexfloat_lower, // 'a' + hexfloat_upper, // 'A' + exp_lower, // 'e' + exp_upper, // 'E' + fixed_lower, // 'f' + fixed_upper, // 'F' + general_lower, // 'g' + general_upper, // 'G' + chr, // 'c' + string, // 's' + pointer // 'p' +}; + // Format specifiers for built-in and string types. template struct basic_format_specs { int width; int precision; - char type; + presentation_type type; align_t align : 4; sign_t sign : 3; bool alt : 1; // Alternate form ('#'). @@ -1963,7 +1985,7 @@ template struct basic_format_specs { constexpr basic_format_specs() : width(0), precision(-1), - type(0), + type(presentation_type::none), align(align::none), sign(sign::none), alt(false), @@ -2043,9 +2065,7 @@ template class specs_setter { } FMT_CONSTEXPR void end_precision() {} - FMT_CONSTEXPR void on_type(Char type) { - specs_.type = static_cast(type); - } + FMT_CONSTEXPR void on_type(presentation_type type) { specs_.type = type; } }; // Format spec handler that saves references to arguments representing dynamic @@ -2327,6 +2347,48 @@ FMT_CONSTEXPR auto parse_precision(const Char* begin, const Char* end, return begin; } +template +FMT_CONSTEXPR auto parse_presentation_type(Char type) -> presentation_type { + switch (to_ascii(type)) { + case 'd': + return presentation_type::dec; + case 'o': + return presentation_type::oct; + case 'x': + return presentation_type::hex_lower; + case 'X': + return presentation_type::hex_upper; + case 'b': + return presentation_type::bin_lower; + case 'B': + return presentation_type::bin_upper; + case 'a': + return presentation_type::hexfloat_lower; + case 'A': + return presentation_type::hexfloat_upper; + case 'e': + return presentation_type::exp_lower; + case 'E': + return presentation_type::exp_upper; + case 'f': + return presentation_type::fixed_lower; + case 'F': + return presentation_type::fixed_upper; + case 'g': + return presentation_type::general_lower; + case 'G': + return presentation_type::general_upper; + case 'c': + return presentation_type::chr; + case 's': + return presentation_type::string; + case 'p': + return presentation_type::pointer; + default: + return presentation_type::none; + } +} + // Parses standard format specifiers and sends notifications about parsed // components to handler. template @@ -2336,7 +2398,10 @@ FMT_CONSTEXPR FMT_INLINE auto parse_format_specs(const Char* begin, -> const Char* { if (begin + 1 < end && begin[1] == '}' && is_ascii_letter(*begin) && *begin != 'L') { - handler.on_type(*begin++); + presentation_type type = parse_presentation_type(*begin++); + if (type == presentation_type::none) + handler.on_error("invalid type specifier"); + handler.on_type(type); return begin; } @@ -2390,7 +2455,12 @@ FMT_CONSTEXPR FMT_INLINE auto parse_format_specs(const Char* begin, } // Parse type. - if (begin != end && *begin != '}') handler.on_type(*begin++); + if (begin != end && *begin != '}') { + presentation_type type = parse_presentation_type(*begin++); + if (type == presentation_type::none) + handler.on_error("invalid type specifier"); + handler.on_type(type); + } return begin; } @@ -2532,28 +2602,18 @@ class compile_parse_context }; template -FMT_CONSTEXPR void check_int_type_spec(char spec, ErrorHandler&& eh) { - switch (spec) { - case 0: - case 'd': - case 'x': - case 'X': - case 'b': - case 'B': - case 'o': - case 'c': - break; - default: +FMT_CONSTEXPR void check_int_type_spec(presentation_type type, + ErrorHandler&& eh) { + if (type > presentation_type::bin_upper && type != presentation_type::chr) eh.on_error("invalid type specifier"); - break; - } } // Checks char specs and returns true if the type spec is char (and not int). template FMT_CONSTEXPR auto check_char_specs(const basic_format_specs& specs, ErrorHandler&& eh = {}) -> bool { - if (specs.type && specs.type != 'c') { + if (specs.type != presentation_type::none && + specs.type != presentation_type::chr) { check_int_type_spec(specs.type, eh); return false; } @@ -2589,33 +2649,33 @@ FMT_CONSTEXPR auto parse_float_type_spec(const basic_format_specs& specs, result.showpoint = specs.alt; result.locale = specs.localized; switch (specs.type) { - case 0: + case presentation_type::none: result.format = float_format::general; break; - case 'G': + case presentation_type::general_upper: result.upper = true; FMT_FALLTHROUGH; - case 'g': + case presentation_type::general_lower: result.format = float_format::general; break; - case 'E': + case presentation_type::exp_upper: result.upper = true; FMT_FALLTHROUGH; - case 'e': + case presentation_type::exp_lower: result.format = float_format::exp; result.showpoint |= specs.precision != 0; break; - case 'F': + case presentation_type::fixed_upper: result.upper = true; FMT_FALLTHROUGH; - case 'f': + case presentation_type::fixed_lower: result.format = float_format::fixed; result.showpoint |= specs.precision != 0; break; - case 'A': + case presentation_type::hexfloat_upper: result.upper = true; FMT_FALLTHROUGH; - case 'a': + case presentation_type::hexfloat_lower: result.format = float_format::hex; break; default: @@ -2625,22 +2685,27 @@ FMT_CONSTEXPR auto parse_float_type_spec(const basic_format_specs& specs, return result; } -template -FMT_CONSTEXPR auto check_cstring_type_spec(Char spec, ErrorHandler&& eh = {}) - -> bool { - if (spec == 0 || spec == 's') return true; - if (spec != 'p') eh.on_error("invalid type specifier"); +template +FMT_CONSTEXPR auto check_cstring_type_spec(presentation_type type, + ErrorHandler&& eh = {}) -> bool { + if (type == presentation_type::none || type == presentation_type::string) + return true; + if (type != presentation_type::pointer) eh.on_error("invalid type specifier"); return false; } -template -FMT_CONSTEXPR void check_string_type_spec(Char spec, ErrorHandler&& eh = {}) { - if (spec != 0 && spec != 's') eh.on_error("invalid type specifier"); +template +FMT_CONSTEXPR void check_string_type_spec(presentation_type type, + ErrorHandler&& eh = {}) { + if (type != presentation_type::none && type != presentation_type::string) + eh.on_error("invalid type specifier"); } -template -FMT_CONSTEXPR void check_pointer_type_spec(Char spec, ErrorHandler&& eh) { - if (spec != 0 && spec != 'p') eh.on_error("invalid type specifier"); +template +FMT_CONSTEXPR void check_pointer_type_spec(presentation_type type, + ErrorHandler&& eh) { + if (type != presentation_type::none && type != presentation_type::pointer) + eh.on_error("invalid type specifier"); } // A parse_format_specs handler that checks if specifiers are consistent with @@ -2818,7 +2883,10 @@ struct formatter arg, static_assert(std::is_same>::value, ""); auto abs_value = arg.abs_value; auto prefix = arg.prefix; - auto utype = static_cast(specs.type); - switch (specs.type) { - case 0: - case 'd': { + auto pres_type = static_cast(specs.type); + switch (pres_type) { + case presentation_type::none: + case presentation_type::dec: { if (specs.localized && write_int_localized(out, static_cast>(abs_value), prefix, specs, loc)) { @@ -1573,38 +1573,40 @@ FMT_CONSTEXPR FMT_INLINE auto write_int(OutputIt out, write_int_arg arg, return format_decimal(it, abs_value, num_digits).end; }); } - case 'x': - case 'X': { - if (specs.alt) prefix_append(prefix, (utype << 8) | '0'); - bool upper = specs.type != 'x'; + case presentation_type::hex_lower: + case presentation_type::hex_upper: { + bool upper = pres_type == presentation_type::hex_upper; + if (specs.alt) + prefix_append(prefix, unsigned(upper ? 'X' : 'x') << 8 | '0'); int num_digits = count_digits<4>(abs_value); return write_int( out, num_digits, prefix, specs, [=](reserve_iterator it) { return format_uint<4, Char>(it, abs_value, num_digits, upper); }); } - case 'b': - case 'B': { - if (specs.alt) prefix_append(prefix, (utype << 8) | '0'); + case presentation_type::bin_lower: + case presentation_type::bin_upper: { + bool upper = pres_type == presentation_type::bin_upper; + if (specs.alt) + prefix_append(prefix, unsigned(upper ? 'B' : 'b') << 8 | '0'); int num_digits = count_digits<1>(abs_value); return write_int(out, num_digits, prefix, specs, [=](reserve_iterator it) { return format_uint<1, Char>(it, abs_value, num_digits); }); } - case 'o': { + case presentation_type::oct: { int num_digits = count_digits<3>(abs_value); - if (specs.alt && specs.precision <= num_digits && abs_value != 0) { - // Octal prefix '0' is counted as a digit, so only add it if precision - // is not greater than the number of digits. + // Octal prefix '0' is counted as a digit, so only add it if precision + // is not greater than the number of digits. + if (specs.alt && specs.precision <= num_digits && abs_value != 0) prefix_append(prefix, '0'); - } return write_int(out, num_digits, prefix, specs, [=](reserve_iterator it) { return format_uint<3, Char>(it, abs_value, num_digits); }); } - case 'c': + case presentation_type::chr: return write_char(out, static_cast(abs_value), specs); default: FMT_THROW(format_error("invalid type specifier")); @@ -1924,7 +1926,9 @@ auto write(OutputIt out, T value, basic_format_specs specs, return write_bytes(out, {buffer.data(), buffer.size()}, specs); } - int precision = specs.precision >= 0 || !specs.type ? specs.precision : 6; + int precision = specs.precision >= 0 || specs.type == presentation_type::none + ? specs.precision + : 6; if (fspecs.format == float_format::exp) { if (precision == max_value()) FMT_THROW(format_error("number is too big")); @@ -2032,7 +2036,8 @@ template & specs = {}, locale_ref = {}) -> OutputIt { - return specs.type && specs.type != 's' + return specs.type != presentation_type::none && + specs.type != presentation_type::string ? write(out, value ? 1 : 0, specs, {}) : write_bytes(out, value ? "true" : "false", specs); } diff --git a/include/fmt/printf.h b/include/fmt/printf.h index 3a3cd152..19d550f6 100644 --- a/include/fmt/printf.h +++ b/include/fmt/printf.h @@ -233,7 +233,7 @@ class printf_arg_formatter : public arg_formatter { OutputIt write_null_pointer(bool is_string = false) { auto s = this->specs; - s.type = 0; + s.type = presentation_type::none; return write_bytes(this->out, is_string ? "(null)" : "(nil)", s); } @@ -249,8 +249,10 @@ class printf_arg_formatter : public arg_formatter { // std::is_same instead. if (std::is_same::value) { format_specs fmt_specs = this->specs; - if (fmt_specs.type && fmt_specs.type != 'c') + if (fmt_specs.type != presentation_type::none && + fmt_specs.type != presentation_type::chr) { return (*this)(static_cast(value)); + } fmt_specs.sign = sign::none; fmt_specs.alt = false; fmt_specs.fill[0] = ' '; // Ignore '0' flag for char types. @@ -271,13 +273,13 @@ class printf_arg_formatter : public arg_formatter { /** Formats a null-terminated C string. */ OutputIt operator()(const char* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs.type != 'p'); + return write_null_pointer(this->specs.type != presentation_type::pointer); } /** Formats a null-terminated wide C string. */ OutputIt operator()(const wchar_t* value) { if (value) return base::operator()(value); - return write_null_pointer(this->specs.type != 'p'); + return write_null_pointer(this->specs.type != presentation_type::pointer); } OutputIt operator()(basic_string_view value) { @@ -490,13 +492,13 @@ void vprintf(buffer& buf, basic_string_view format, // Parse type. if (it == end) FMT_THROW(format_error("invalid format string")); - specs.type = static_cast(*it++); + char type = static_cast(*it++); if (arg.is_integral()) { // Normalize type. - switch (specs.type) { + switch (type) { case 'i': case 'u': - specs.type = 'd'; + type = 'd'; break; case 'c': visit_format_arg( @@ -505,6 +507,9 @@ void vprintf(buffer& buf, basic_string_view format, break; } } + specs.type = parse_presentation_type(type); + if (specs.type == presentation_type::none) + parse_ctx.on_error("invalid type specifier"); start = it; diff --git a/test/core-test.cc b/test/core-test.cc index 44acbe37..c99098cb 100644 --- a/test/core-test.cc +++ b/test/core-test.cc @@ -524,7 +524,7 @@ struct test_format_specs_handler { fmt::detail::arg_ref width_ref; int precision = 0; fmt::detail::arg_ref precision_ref; - char type = 0; + fmt::presentation_type type = fmt::presentation_type::none; // Workaround for MSVC2017 bug that results in "expression did not evaluate // to a constant" with compiler-generated copy ctor. @@ -550,14 +550,14 @@ struct test_format_specs_handler { constexpr void on_dynamic_precision(string_view) {} constexpr void end_precision() {} - constexpr void on_type(char t) { type = t; } + constexpr void on_type(fmt::presentation_type t) { type = t; } constexpr void on_error(const char*) { res = error; } }; template constexpr test_format_specs_handler parse_test_specs(const char (&s)[N]) { auto h = test_format_specs_handler(); - fmt::detail::parse_format_specs(s, s + N, h); + fmt::detail::parse_format_specs(s, s + N - 1, h); return h; } @@ -575,7 +575,7 @@ TEST(core_test, constexpr_parse_format_specs) { static_assert(parse_test_specs("{42}").width_ref.val.index == 42, ""); static_assert(parse_test_specs(".42").precision == 42, ""); static_assert(parse_test_specs(".{42}").precision_ref.val.index == 42, ""); - static_assert(parse_test_specs("d").type == 'd', ""); + static_assert(parse_test_specs("d").type == fmt::presentation_type::dec, ""); static_assert(parse_test_specs("{<").res == handler::error, ""); } @@ -597,7 +597,7 @@ constexpr fmt::detail::dynamic_format_specs parse_dynamic_specs( auto specs = fmt::detail::dynamic_format_specs(); auto ctx = test_parse_context(); auto h = fmt::detail::dynamic_specs_handler(specs, ctx); - parse_format_specs(s, s + N, h); + parse_format_specs(s, s + N - 1, h); return specs; } @@ -615,14 +615,15 @@ TEST(format_test, constexpr_dynamic_specs_handler) { static_assert(parse_dynamic_specs(".42").precision == 42, ""); static_assert(parse_dynamic_specs(".{}").precision_ref.val.index == 11, ""); static_assert(parse_dynamic_specs(".{42}").precision_ref.val.index == 42, ""); - static_assert(parse_dynamic_specs("d").type == 'd', ""); + static_assert(parse_dynamic_specs("d").type == fmt::presentation_type::dec, + ""); } template constexpr test_format_specs_handler check_specs(const char (&s)[N]) { fmt::detail::specs_checker checker( test_format_specs_handler(), fmt::detail::type::double_type); - parse_format_specs(s, s + N, checker); + parse_format_specs(s, s + N - 1, checker); return checker; } @@ -639,7 +640,7 @@ TEST(format_test, constexpr_specs_checker) { static_assert(check_specs("{42}").width_ref.val.index == 42, ""); static_assert(check_specs(".42").precision == 42, ""); static_assert(check_specs(".{42}").precision_ref.val.index == 42, ""); - static_assert(check_specs("d").type == 'd', ""); + static_assert(check_specs("d").type == fmt::presentation_type::dec, ""); static_assert(check_specs("{<").res == handler::error, ""); } diff --git a/test/format-test.cc b/test/format-test.cc index d6c68922..41303c93 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -555,7 +555,7 @@ TEST(format_test, fill) { fmt::format(string_view("{:\0>4}", 6), '*')); EXPECT_EQ("жж42", fmt::format("{0:ж>4}", 42)); EXPECT_THROW_MSG(fmt::format(runtime("{:\x80\x80\x80\x80\x80>}"), 0), - format_error, "missing '}' in format string"); + format_error, "invalid type specifier"); } TEST(format_test, plus_sign) { @@ -1037,7 +1037,7 @@ void check_unknown_types(const T& value, const char* types, const char*) { TEST(format_test, format_int) { EXPECT_THROW_MSG(fmt::format(runtime("{0:v"), 42), format_error, - "missing '}' in format string"); + "invalid type specifier"); check_unknown_types(42, "bBdoxXnLc", "integer"); EXPECT_EQ("x", fmt::format("{:c}", static_cast('x'))); } diff --git a/test/fuzzing/named-arg.cc b/test/fuzzing/named-arg.cc index 97085dab..3bee1ae3 100644 --- a/test/fuzzing/named-arg.cc +++ b/test/fuzzing/named-arg.cc @@ -29,7 +29,8 @@ void invoke_fmt(const uint8_t* data, size_t size, unsigned arg_name_size) { fmt::format(format_str.get(), fmt::arg(arg_name.data(), value)); #else fmt::memory_buffer out; - fmt::format_to(out, format_str.get(), fmt::arg(arg_name.data(), value)); + fmt::format_to(std::back_inserter(out), format_str.get(), + fmt::arg(arg_name.data(), value)); #endif } catch (std::exception&) { } diff --git a/test/xchar-test.cc b/test/xchar-test.cc index 106840e6..ca11d7a7 100644 --- a/test/xchar-test.cc +++ b/test/xchar-test.cc @@ -409,7 +409,7 @@ template struct formatter, charT> { specs_.precision, specs_.precision_ref, ctx); auto specs = std::string(); if (specs_.precision > 0) specs = fmt::format(".{}", specs_.precision); - if (specs_.type) specs += specs_.type; + if (specs_.type == presentation_type::fixed_lower) specs += 'f'; auto real = fmt::format(ctx.locale().template get(), fmt::runtime("{:" + specs + "}"), c.real()); auto imag = fmt::format(ctx.locale().template get(),