From dafbec7553337717c932436ad17569662256aec9 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Fri, 7 Oct 2016 08:37:06 -0700 Subject: [PATCH] Fix type safety when using custom formatters (#394) --- fmt/format.cc | 64 +---------------- fmt/format.h | 128 ++++++++++++++++++++++++++-------- fmt/printf.h | 40 +++++++---- test/custom-formatter-test.cc | 16 +++-- test/format-test.cc | 9 ++- test/ostream-test.cc | 2 +- 6 files changed, 143 insertions(+), 116 deletions(-) diff --git a/fmt/format.cc b/fmt/format.cc index 6abda6d5..2f43762f 100644 --- a/fmt/format.cc +++ b/fmt/format.cc @@ -403,72 +403,11 @@ FMT_FUNC void format_system_error( fmt::format_error_code(out, error_code, message); // 'fmt::' is for bcc32. } -template -void internal::ArgMap::init(const format_args &args) { - if (!map_.empty()) - return; - typedef internal::NamedArg NamedArg; - const NamedArg *named_arg = 0; - bool use_values = - args.type(MAX_PACKED_ARGS - 1) == internal::Arg::NONE; - if (use_values) { - for (unsigned i = 0;/*nothing*/; ++i) { - internal::Arg::Type arg_type = args.type(i); - switch (arg_type) { - case internal::Arg::NONE: - return; - case internal::Arg::NAMED_ARG: - named_arg = static_cast(args.values_[i].pointer); - map_.push_back(Pair(named_arg->name, *named_arg)); - break; - default: - /*nothing*/; - } - } - return; - } - for (unsigned i = 0; i != MAX_PACKED_ARGS; ++i) { - internal::Arg::Type arg_type = args.type(i); - if (arg_type == internal::Arg::NAMED_ARG) { - named_arg = static_cast(args.args_[i].pointer); - map_.push_back(Pair(named_arg->name, *named_arg)); - } - } - for (unsigned i = MAX_PACKED_ARGS;/*nothing*/; ++i) { - switch (args.args_[i].type) { - case internal::Arg::NONE: - return; - case internal::Arg::NAMED_ARG: - named_arg = static_cast(args.args_[i].pointer); - map_.push_back(Pair(named_arg->name, *named_arg)); - break; - default: - /*nothing*/; - } - } -} - template void internal::FixedBuffer::grow(std::size_t) { FMT_THROW(std::runtime_error("buffer overflow")); } -FMT_FUNC Arg internal::FormatterBase::do_get_arg( - unsigned arg_index, const char *&error) { - Arg arg = args_[arg_index]; - switch (arg.type) { - case Arg::NONE: - error = "argument index out of range"; - break; - case Arg::NAMED_ARG: - arg = *static_cast(arg.pointer); - break; - default: - /*nothing*/; - } - return arg; -} - FMT_FUNC void report_system_error( int error_code, fmt::StringRef message) FMT_NOEXCEPT { // 'fmt::' is for bcc32. @@ -505,7 +444,8 @@ template void printf(BasicWriter &w, BasicCStringRef format, format_args args); -FMT_FUNC int vfprintf(std::FILE *f, CStringRef format, format_args args) { +FMT_FUNC int vfprintf(std::FILE *f, CStringRef format, + basic_format_args> args) { MemoryWriter w; printf(w, format, args); std::size_t size = w.size(); diff --git a/fmt/format.h b/fmt/format.h index 27137d4d..e012bba1 100644 --- a/fmt/format.h +++ b/fmt/format.h @@ -1429,7 +1429,7 @@ constexpr uint64_t make_type() { return 0; } enum { MAX_PACKED_ARGS = 16 }; } // namespace internal -template +template class format_arg_store { private: static const size_t NUM_ARGS = sizeof...(Args); @@ -1441,13 +1441,12 @@ class format_arg_store { // If the arguments are not packed, add one more element to mark the end. std::array data_; - template - friend format_arg_store make_format_args(const A & ... args); + template + friend format_arg_store make_format_args(const A & ... args); public: static const uint64_t TYPES = internal::make_type(); - template format_arg_store(const Args &... args, Formatter *) : data_{{internal::MakeValue(args)...}} {} @@ -1455,13 +1454,15 @@ class format_arg_store { }; template -inline format_arg_store make_format_args(const Args & ... args) { +inline format_arg_store + make_format_args(const Args & ... args) { Formatter *f = nullptr; - return format_arg_store(args..., f); + return format_arg_store(args..., f); } /** Formatting arguments. */ -class format_args { +template +class basic_format_args { private: // To reduce compiled code size per formatting function call, types of first // MAX_PACKED_ARGS arguments are passed in the types_ field. @@ -1491,10 +1492,10 @@ class format_args { public: typedef unsigned size_type; - format_args() : types_(0) {} + basic_format_args() : types_(0) {} - template - format_args(const format_arg_store &store) + template + basic_format_args(const format_arg_store &store) : types_(store.TYPES) { set_data(store.data()); } @@ -1525,6 +1526,9 @@ class format_args { } }; +typedef basic_format_args> format_args; +typedef basic_format_args> wformat_args; + #define FMT_DISPATCH(call) static_cast(this)->call /** @@ -1919,7 +1923,8 @@ class ArgMap { MapType map_; public: - FMT_API void init(const format_args &args); + template + void init(const basic_format_args &args); const internal::Arg* find(const fmt::BasicStringRef &name) const { // The list is unsorted, so just return the first matching name. @@ -1932,6 +1937,52 @@ class ArgMap { } }; +template +template +void ArgMap::init(const basic_format_args &args) { + if (!map_.empty()) + return; + typedef internal::NamedArg NamedArg; + const NamedArg *named_arg = 0; + bool use_values = + args.type(MAX_PACKED_ARGS - 1) == internal::Arg::NONE; + if (use_values) { + for (unsigned i = 0;/*nothing*/; ++i) { + internal::Arg::Type arg_type = args.type(i); + switch (arg_type) { + case internal::Arg::NONE: + return; + case internal::Arg::NAMED_ARG: + named_arg = static_cast(args.values_[i].pointer); + map_.push_back(Pair(named_arg->name, *named_arg)); + break; + default: + /*nothing*/; + } + } + return; + } + for (unsigned i = 0; i != MAX_PACKED_ARGS; ++i) { + internal::Arg::Type arg_type = args.type(i); + if (arg_type == internal::Arg::NAMED_ARG) { + named_arg = static_cast(args.args_[i].pointer); + map_.push_back(Pair(named_arg->name, *named_arg)); + } + } + for (unsigned i = MAX_PACKED_ARGS;/*nothing*/; ++i) { + switch (args.args_[i].type) { + case internal::Arg::NONE: + return; + case internal::Arg::NAMED_ARG: + named_arg = static_cast(args.args_[i].pointer); + map_.push_back(Pair(named_arg->name, *named_arg)); + break; + default: + /*nothing*/; + } + } +} + template class ArgFormatterBase : public ArgVisitor { private: @@ -2030,21 +2081,33 @@ class ArgFormatterBase : public ArgVisitor { } }; +template class FormatterBase { - private: - format_args args_; +private: + basic_format_args args_; int next_arg_index_; // Returns the argument with specified index. - FMT_API Arg do_get_arg(unsigned arg_index, const char *&error); + Arg do_get_arg(unsigned arg_index, const char *&error) { + Arg arg = args_[arg_index]; + switch (arg.type) { + case Arg::NONE: + error = "argument index out of range"; + break; + case Arg::NAMED_ARG: + arg = *static_cast(arg.pointer); + break; + default: + /*nothing*/; + } + return arg; + } protected: - const format_args &args() const { return args_; } + FormatterBase(basic_format_args args) + : args_(args), next_arg_index_(0) {} - explicit FormatterBase(const format_args &args) { - args_ = args; - next_arg_index_ = 0; - } + const basic_format_args &args() const { return args_; } // Returns the next argument. Arg next_arg(const char *&error) { @@ -2132,7 +2195,8 @@ class ArgFormatter : public BasicArgFormatter, Char> { /** This template formats data and writes the output to a writer. */ template -class BasicFormatter : private internal::FormatterBase { +class BasicFormatter : + private internal::FormatterBase> { public: /** The character type for the output. */ typedef CharType Char; @@ -2143,7 +2207,8 @@ class BasicFormatter : private internal::FormatterBase { FMT_DISALLOW_COPY_AND_ASSIGN(BasicFormatter); - using internal::FormatterBase::get_arg; + typedef internal::FormatterBase Base; + using Base::get_arg; // Checks if manual indexing is used and returns the argument with // specified name. @@ -2163,8 +2228,8 @@ class BasicFormatter : private internal::FormatterBase { appropriate lifetimes. \endrst */ - BasicFormatter(const format_args &args, BasicWriter &w) - : internal::FormatterBase(args), writer_(w) {} + BasicFormatter(basic_format_args args, BasicWriter &w) + : Base(args), writer_(w) {} /** Returns a reference to the writer associated with this formatter. */ BasicWriter &writer() { return writer_; } @@ -2404,7 +2469,8 @@ class BasicWriter { return std::basic_string(&buffer_[0], buffer_.size()); } - void vwrite(BasicCStringRef format, format_args args) { + void vwrite(BasicCStringRef format, + basic_format_args> args) { BasicFormatter(args, *this).format(format); } @@ -3111,7 +3177,7 @@ inline std::string format(CStringRef format_str, const Args & ... args) { return vformat(format_str, make_format_args>(args...)); } -inline std::wstring vformat(WCStringRef format_str, format_args args) { +inline std::wstring vformat(WCStringRef format_str, wformat_args args) { WMemoryWriter w; w.vwrite(format_str, args); return w.str(); @@ -3348,8 +3414,8 @@ void check_sign(const Char *&s, const Arg &arg) { template inline internal::Arg BasicFormatter::get_arg( BasicStringRef arg_name, const char *&error) { - if (check_no_auto_index(error)) { - map_.init(args()); + if (this->check_no_auto_index(error)) { + map_.init(this->args()); const internal::Arg *arg = map_.find(arg_name); if (arg) return *arg; @@ -3362,7 +3428,7 @@ template inline internal::Arg BasicFormatter::parse_arg_index(const Char *&s) { const char *error = 0; internal::Arg arg = *s < '0' || *s > '9' ? - next_arg(error) : get_arg(internal::parse_nonnegative_int(s), error); + this->next_arg(error) : get_arg(internal::parse_nonnegative_int(s), error); if (error) { FMT_THROW(format_error( *s != '}' && *s != ':' ? "invalid format string" : error)); @@ -3563,18 +3629,18 @@ void BasicFormatter::format(BasicCStringRef format_str) { Char c = *s++; if (c != '{' && c != '}') continue; if (*s == c) { - write(writer_, start, s); + this->write(writer_, start, s); start = ++s; continue; } if (c == '}') FMT_THROW(format_error("unmatched '}' in format string")); - write(writer_, start, s - 1); + this->write(writer_, start, s - 1); internal::Arg arg = internal::is_name_start(*s) ? parse_arg_name(s) : parse_arg_index(s); start = s = format(s, arg); } - write(writer_, start, s); + this->write(writer_, start, s); } } // namespace fmt diff --git a/fmt/printf.h b/fmt/printf.h index 47f71c9b..4317f10b 100644 --- a/fmt/printf.h +++ b/fmt/printf.h @@ -262,7 +262,8 @@ class BasicPrintfArgFormatter : public internal::ArgFormatterBase { /** Formats an argument of a custom (user-defined) type. */ void visit_custom(internal::Arg::CustomValue c) { - BasicFormatter formatter(format_args(), this->writer()); + BasicFormatter formatter(basic_format_args>(), + this->writer()); const Char format_str[] = {'}', 0}; const Char *format = format_str; c.format(&formatter, c.value, &format); @@ -281,10 +282,13 @@ class PrintfArgFormatter /** This template formats data and writes the output to a writer. */ template > -class PrintfFormatter : private internal::FormatterBase { +class PrintfFormatter : + private internal::FormatterBase> { private: BasicWriter &writer_; + typedef internal::FormatterBase Base; + void parse_flags(FormatSpec &spec, const Char *&s); // Returns the argument with specified index or, if arg_index is equal @@ -304,8 +308,9 @@ class PrintfFormatter : private internal::FormatterBase { appropriate lifetimes. \endrst */ - explicit PrintfFormatter(const format_args &args, BasicWriter &w) - : FormatterBase(args), writer_(w) {} + explicit PrintfFormatter(basic_format_args args, + BasicWriter &w) + : Base(args), writer_(w) {} /** Formats stored arguments and writes the output to the writer. */ FMT_API void format(BasicCStringRef format_str); @@ -343,7 +348,7 @@ internal::Arg PrintfFormatter::get_arg(const Char *s, (void)s; const char *error = 0; internal::Arg arg = arg_index == std::numeric_limits::max() ? - next_arg(error) : FormatterBase::get_arg(arg_index - 1, error); + this->next_arg(error) : Base::get_arg(arg_index - 1, error); if (error) FMT_THROW(format_error(!*s ? "invalid format string" : error)); return arg; @@ -391,11 +396,11 @@ void PrintfFormatter::format(BasicCStringRef format_str) { Char c = *s++; if (c != '%') continue; if (*s == c) { - write(writer_, start, s); + this->write(writer_, start, s); start = ++s; continue; } - write(writer_, start, s - 1); + this->write(writer_, start, s - 1); FormatSpec spec; spec.align_ = ALIGN_RIGHT; @@ -480,16 +485,17 @@ void PrintfFormatter::format(BasicCStringRef format_str) { // Format argument. AF(writer_, spec).visit(arg); } - write(writer_, start, s); + this->write(writer_, start, s); } template void printf(BasicWriter &w, BasicCStringRef format, - format_args args) { + basic_format_args> args) { PrintfFormatter(args, w).format(format); } -inline std::string vsprintf(CStringRef format, format_args args) { +inline std::string vsprintf(CStringRef format, + basic_format_args> args) { MemoryWriter w; printf(w, format, args); return w.str(); @@ -509,7 +515,8 @@ inline std::string sprintf(CStringRef format_str, const Args & ... args) { return vsprintf(format_str, make_format_args>(args...)); } -inline std::wstring vsprintf(WCStringRef format, format_args args) { +inline std::wstring vsprintf(WCStringRef format, + basic_format_args> args) { WMemoryWriter w; printf(w, format, args); return w.str(); @@ -521,7 +528,8 @@ inline std::wstring sprintf(WCStringRef format_str, const Args & ... args) { return vsprintf(format_str, vargs); } -FMT_API int vfprintf(std::FILE *f, CStringRef format, format_args args); +FMT_API int vfprintf(std::FILE *f, CStringRef format, + basic_format_args> args); /** \rst @@ -534,11 +542,12 @@ FMT_API int vfprintf(std::FILE *f, CStringRef format, format_args args); */ template inline int fprintf(std::FILE *f, CStringRef format_str, const Args & ... args) { - auto vargs = make_format_args>(args...); + auto vargs = make_format_args>(args...); return vfprintf(f, format_str, vargs); } -inline int vprintf(CStringRef format, format_args args) { +inline int vprintf(CStringRef format, + basic_format_args> args) { return vfprintf(stdout, format, args); } @@ -556,7 +565,8 @@ inline int printf(CStringRef format_str, const Args & ... args) { return vprintf(format_str, make_format_args>(args...)); } -inline int vfprintf(std::ostream &os, CStringRef format_str, format_args args) { +inline int vfprintf(std::ostream &os, CStringRef format_str, + basic_format_args> args) { MemoryWriter w; printf(w, format_str, args); internal::write(os, w); diff --git a/test/custom-formatter-test.cc b/test/custom-formatter-test.cc index 32ea166e..b556ee87 100644 --- a/test/custom-formatter-test.cc +++ b/test/custom-formatter-test.cc @@ -45,10 +45,13 @@ class CustomPrintfArgFormatter : } }; -std::string custom_vformat(const char *format_str, fmt::format_args args) { +typedef fmt::BasicFormatter CustomFormatter; + +std::string custom_vformat(const char *format_str, + fmt::basic_format_args args) { fmt::MemoryWriter writer; // Pass custom argument formatter as a template arg to BasicFormatter. - fmt::BasicFormatter formatter(args, writer); + CustomFormatter formatter(args, writer); formatter.format(format_str); return writer.str(); } @@ -59,9 +62,14 @@ std::string custom_format(const char *format_str, const Args & ... args) { return custom_vformat(format_str, va); } -std::string custom_vsprintf(const char* format_str, fmt::format_args args) { +typedef fmt::PrintfFormatter + CustomPrintfFormatter; + +std::string custom_vsprintf( + const char* format_str, + fmt::basic_format_args args) { fmt::MemoryWriter writer; - fmt::PrintfFormatter formatter(args, writer); + CustomPrintfFormatter formatter(args, writer); formatter.format(format_str); return writer.str(); } diff --git a/test/format-test.cc b/test/format-test.cc index 022b1aa7..616c015d 100644 --- a/test/format-test.cc +++ b/test/format-test.cc @@ -1633,16 +1633,19 @@ class MockArgFormatter : MOCK_METHOD1(visit_int, void (int value)); }; -void vcustom_format(const char *format_str, fmt::format_args args) { +typedef fmt::BasicFormatter CustomFormatter; + +void custom_vformat(const char *format_str, + fmt::basic_format_args args) { fmt::MemoryWriter writer; - fmt::BasicFormatter formatter(args, writer); + CustomFormatter formatter(args, writer); formatter.format(format_str); } template void custom_format(const char *format_str, const Args & ... args) { auto va = fmt::make_format_args>(args...); - return vcustom_format(format_str, va); + return custom_vformat(format_str, va); } TEST(FormatTest, CustomArgFormatter) { diff --git a/test/ostream-test.cc b/test/ostream-test.cc index 9542a28c..2469e041 100644 --- a/test/ostream-test.cc +++ b/test/ostream-test.cc @@ -67,7 +67,7 @@ struct TestArgFormatter : fmt::BasicArgFormatter { TEST(OStreamTest, CustomArg) { fmt::MemoryWriter writer; typedef fmt::BasicFormatter Formatter; - Formatter formatter(fmt::format_args(), writer); + Formatter formatter(fmt::basic_format_args(), writer); fmt::FormatSpec spec; TestArgFormatter af(formatter, spec, "}"); af.visit(fmt::internal::MakeArg(TestEnum()));