From 9f70fc3e7a1725c50de294f8f966789bf6796099 Mon Sep 17 00:00:00 2001 From: Victor Zverovich Date: Mon, 16 Mar 2020 07:51:57 -0700 Subject: [PATCH] Minor tweaks for dynamic_format_arg_store --- include/fmt/core.h | 333 +++++++++++++++++------------------ test/CMakeLists.txt | 1 - test/core-test.cc | 80 ++++++++- test/format-dyn-args-test.cc | 81 --------- 4 files changed, 236 insertions(+), 259 deletions(-) diff --git a/include/fmt/core.h b/include/fmt/core.h index 227d15f7..8dc770b4 100644 --- a/include/fmt/core.h +++ b/include/fmt/core.h @@ -1205,6 +1205,62 @@ template make_arg(const T& value) { return make_arg(value); } + +template struct is_string_view : std::false_type {}; + +template +struct is_string_view, Char> : std::true_type {}; + +template +struct is_string_view, Char> : std::true_type {}; + +template struct is_reference_wrapper : std::false_type {}; + +template +struct is_reference_wrapper> : std::true_type {}; + +class dyn_arg_storage { + // Workaround for clang's -Wweak-vtables. Unlike for regular classes, for + // templates it doesn't complain about inability to deduce single translation + // unit for placing vtable. So storage_node_base is made a fake template. + template struct storage_node_base { + using owning_ptr = std::unique_ptr>; + virtual ~storage_node_base() = default; + owning_ptr next; + }; + + template struct storage_node : storage_node_base<> { + T value; + + template + FMT_CONSTEXPR storage_node(const Arg& arg, owning_ptr&& next) : value(arg) { + this->next = std::move(next); // Must be initialised after value. + } + + template + FMT_CONSTEXPR storage_node(const basic_string_view& arg, + owning_ptr&& next) + : value(arg.data(), arg.size()) { + this->next = std::move(next); // Must be initialised after value. + } + }; + + storage_node_base<>::owning_ptr head_{nullptr}; + + public: + dyn_arg_storage() = default; + dyn_arg_storage(const dyn_arg_storage&) = delete; + dyn_arg_storage(dyn_arg_storage&&) = default; + + dyn_arg_storage& operator=(const dyn_arg_storage&) = delete; + dyn_arg_storage& operator=(dyn_arg_storage&&) = default; + + template const T& push(const Arg& arg) { + auto node = new storage_node(arg, std::move(head_)); + head_.reset(node); + return node->value; + } +}; } // namespace internal // Formatting context. @@ -1314,7 +1370,108 @@ inline format_arg_store make_format_args( return {args...}; } -template class dynamic_format_arg_store; +/** + \rst + A dynamic version of `fmt::format_arg_store<>`. + It's equipped with a storage to potentially temporary objects which lifetime + could be shorter than the format arguments object. + + It can be implicitly converted into `~fmt::basic_format_args` for passing + into type-erased formatting functions such as `~fmt::vformat`. + \endrst + */ +template +class dynamic_format_arg_store +#if FMT_GCC_VERSION < 409 + // Workaround a GCC template argument substitution bug. + : public basic_format_args +#endif +{ + private: + using char_type = typename Context::char_type; + + template struct need_dyn_copy { + static constexpr internal::type mapped_type = + internal::mapped_type_constant::value; + + using type = std::integral_constant< + bool, !(internal::is_reference_wrapper::value || + internal::is_string_view::value || + (mapped_type != internal::type::cstring_type && + mapped_type != internal::type::string_type && + mapped_type != internal::type::custom_type && + mapped_type != internal::type::named_arg_type))>; + }; + + template + using stored_type = conditional_t::value, + std::basic_string, T>; + + // Storage of basic_format_arg must be contiguous. + std::vector> data_; + + // Storage of arguments not fitting into basic_format_arg must grow + // without relocation because items in data_ refer to it. + internal::dyn_arg_storage storage_; + + friend class basic_format_args; + + unsigned long long get_types() const { + return internal::is_unpacked_bit | data_.size(); + } + + template void emplace_arg(const T& arg) { + data_.emplace_back(internal::make_arg(arg)); + } + + public: + dynamic_format_arg_store() = default; + ~dynamic_format_arg_store() = default; + + dynamic_format_arg_store(const dynamic_format_arg_store&) = delete; + dynamic_format_arg_store& operator=(const dynamic_format_arg_store&) = delete; + + dynamic_format_arg_store(dynamic_format_arg_store&&) = default; + dynamic_format_arg_store& operator=(dynamic_format_arg_store&&) = default; + + /** + \rst + Adds an argument into the dynamic store for later passing to a formating + function. + + Note that custom types and string types (but not string views!) are copied + into the store with dynamic memory (in addition to resizing vector). + + **Example**:: + + fmt::dynamic_format_arg_store store; + store.push_back(42); + store.push_back("abc"); + store.push_back(1.5f); + std::string result = fmt::vformat("{} and {} and {}", store); + \endrst + */ + template void push_back(const T& arg) { + static_assert( + !std::is_base_of, T>::value, + "named arguments are not supported yet"); + if (internal::const_check(need_dyn_copy::type::value)) + emplace_arg(storage_.push>(arg)); + else + emplace_arg(arg); + } + + /** + Adds a reference to the argument into the dynamic store for later passing to + a formating function. + */ + template void push_back(std::reference_wrapper arg) { + static_assert( + need_dyn_copy::type::value, + "objects of built-in types and string views are always copied"); + emplace_arg(arg.get()); + } +}; /** \rst @@ -1639,180 +1796,6 @@ inline void print(const S& format_str, Args&&... args) { internal::make_args_checked(format_str, args...)); #endif } - -namespace internal { - -template struct is_string_view : std::false_type {}; - -template -struct is_string_view, Char> : std::true_type {}; - -template -struct is_string_view, Char> : std::true_type {}; - -template struct is_reference_wrapper : std::false_type {}; - -template -struct is_reference_wrapper> : std::true_type {}; - -class dyn_arg_storage { - // Workaround for clang's -Wweak-vtables. Unlike for regular classes, for - // templates it doesn't complain about inability to deduce single translation - // unit for placing vtable. So storage_node_base is made a fake template. - - template struct storage_node_base { - using owning_ptr = std::unique_ptr>; - virtual ~storage_node_base() = default; - owning_ptr next; - }; - - template struct storage_node : storage_node_base<> { - T value; - template - FMT_CONSTEXPR storage_node(const Arg& arg, owning_ptr&& next) : value{arg} { - // Must be initialised after value_ - this->next = std::move(next); - } - - template - FMT_CONSTEXPR storage_node(const basic_string_view& arg, - owning_ptr&& next) - : value{arg.data(), arg.size()} { - // Must be initialised after value - this->next = std::move(next); - } - }; - - storage_node_base<>::owning_ptr head_{nullptr}; - - public: - dyn_arg_storage() = default; - dyn_arg_storage(const dyn_arg_storage&) = delete; - dyn_arg_storage(dyn_arg_storage&&) = default; - - dyn_arg_storage& operator=(const dyn_arg_storage&) = delete; - dyn_arg_storage& operator=(dyn_arg_storage&&) = default; - - template const T& push(const Arg& arg) { - auto node = new storage_node{arg, std::move(head_)}; - head_.reset(node); - return node->value; - } -}; - -} // namespace internal - -/** - \rst - A dynamic version of `fmt::format_arg_store<>`. - It's equipped with a storage to potentially temporary objects which lifetime - could be shorter than the format arguments object. - - It can be implicitly converted into `~fmt::basic_format_args` for passing - into type-erased formatting functions such as `~fmt::vformat`. - \endrst - */ -template -class dynamic_format_arg_store -#if FMT_GCC_VERSION < 409 - // Workaround a GCC template argument substitution bug. - : public basic_format_args -#endif -{ - private: - using char_type = typename Context::char_type; - - template struct need_dyn_copy { - static constexpr internal::type mapped_type = - internal::mapped_type_constant::value; -// static_assert( -// mapped_type != internal::type::named_arg_type, -// "Bug indicator. Named arguments must be processed separately"); - - using type = std::integral_constant< - bool, !(internal::is_reference_wrapper::value || - internal::is_string_view::value || - (mapped_type != internal::type::cstring_type && - mapped_type != internal::type::string_type && - mapped_type != internal::type::custom_type && - mapped_type != internal::type::named_arg_type))>; - }; - - template - using stored_type = conditional_t::value, - std::basic_string, T>; - - // Storage of basic_format_arg must be contiguous - // Required by basic_format_args::args_ which is just a pointer. - std::vector> data_; - - // Storage of arguments not fitting into basic_format_arg must grow - // without relocation because items in data_ refer to it. - - internal::dyn_arg_storage storage_; - - friend class basic_format_args; - - unsigned long long get_types() const { - return internal::is_unpacked_bit | data_.size(); - } - - template void emplace_arg(const T& arg) { - data_.emplace_back(internal::make_arg(arg)); - } - - public: - dynamic_format_arg_store() = default; - ~dynamic_format_arg_store() = default; - - dynamic_format_arg_store(const dynamic_format_arg_store&) = delete; - dynamic_format_arg_store& operator=(const dynamic_format_arg_store&) = delete; - - dynamic_format_arg_store(dynamic_format_arg_store&&) = default; - dynamic_format_arg_store& operator=(dynamic_format_arg_store&&) = default; - - /** - \rst - Adds an argument into the dynamic store for later passing to a formating - function. - - Note that custom types and string types (but not string views!) are copied - into the store with dynamic memory (in addition to resizing vector). - - **Example**:: - - #include - fmt::dynamic_format_arg_store store; - store.push_back(42); - store.push_back("abc1"); - store.push_back(1.5f); - std::string result = fmt::vformat("{} and {} and {}", store); - \endrst - */ - template void push_back(const T& arg) { - static_assert( - !std::is_base_of, T>::value, - "Named arguments are not supported yet"); - if (internal::const_check(need_dyn_copy::type::value)) - emplace_arg(storage_.push>(arg)); - else - emplace_arg(arg); - } - - /** - \rst - Adds an argument into the dynamic store for later passing to a formating - function without copying into type-erasing list. - \endrst - */ - template void push_back(std::reference_wrapper arg) { - static_assert( - need_dyn_copy::type::value, - "Primitive types and string views directly supported by " - "basic_format_arg. Passing them by reference is not allowed"); - emplace_arg(arg.get()); - } -}; FMT_END_NAMESPACE #endif // FMT_CORE_H_ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 23ea9fcf..89176633 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -95,7 +95,6 @@ add_fmt_test(grisu-test) target_compile_definitions(grisu-test PRIVATE FMT_USE_GRISU=1) add_fmt_test(gtest-extra-test) add_fmt_test(format-test mock-allocator.h) -add_fmt_test(format-dyn-args-test) if (MSVC) target_compile_options(format-test PRIVATE /bigobj) endif () diff --git a/test/core-test.cc b/test/core-test.cc index 6a7c0d3b..c2e4593a 100644 --- a/test/core-test.cc +++ b/test/core-test.cc @@ -15,9 +15,8 @@ #include #include -#include "test-assert.h" - #include "gmock.h" +#include "test-assert.h" // Check if fmt/core.h compiles with windows.h included before it. #ifdef _WIN32 @@ -402,6 +401,83 @@ TEST(ArgTest, VisitInvalidArg) { fmt::visit_format_arg(visitor, arg); } +TEST(FormatDynArgsTest, Basic) { + fmt::dynamic_format_arg_store store; + store.push_back(42); + store.push_back("abc1"); + store.push_back(1.5f); + + std::string result = fmt::vformat("{} and {} and {}", store); + EXPECT_EQ("42 and abc1 and 1.5", result); +} + +TEST(FormatDynArgsTest, StringsAndRefs) { + // Unfortunately the tests are compiled with old ABI so strings use COW. + fmt::dynamic_format_arg_store store; + char str[] = "1234567890"; + store.push_back(str); + store.push_back(std::cref(str)); + store.push_back(fmt::string_view{str}); + str[0] = 'X'; + + std::string result = fmt::vformat("{} and {} and {}", store); + EXPECT_EQ("1234567890 and X234567890 and X234567890", result); +} + +struct custom_type { + int i = 0; +}; + +FMT_BEGIN_NAMESPACE +template <> struct formatter { + auto parse(format_parse_context& ctx) const -> decltype(ctx.begin()) { + return ctx.begin(); + } + + template + auto format(const custom_type& p, FormatContext& ctx) -> decltype(format_to( + ctx.out(), std::declval())) { + return format_to(ctx.out(), "cust={}", p.i); + } +}; +FMT_END_NAMESPACE + +TEST(FormatDynArgsTest, CustomFormat) { + fmt::dynamic_format_arg_store store; + custom_type c{}; + store.push_back(c); + ++c.i; + store.push_back(c); + ++c.i; + store.push_back(std::cref(c)); + ++c.i; + + std::string result = fmt::vformat("{} and {} and {}", store); + EXPECT_EQ("cust=0 and cust=1 and cust=3", result); +} + +TEST(FormatDynArgsTest, NamedArgByRef) { + fmt::dynamic_format_arg_store store; + + // Note: fmt::arg() constructs an object which holds a reference + // to its value. It's not an aggregate, so it doesn't extend the + // reference lifetime. As a result, it's a very bad idea passing temporary + // as a named argument value. Only GCC with optimization level >0 + // complains about this. + // + // A real life usecase is when you have both name and value alive + // guarantee their lifetime and thus don't want them to be copied into + // storages. + int a1_val{42}; + auto a1 = fmt::arg("a1_", a1_val); + store.push_back(std::cref(a1)); + + std::string result = fmt::vformat("{a1_}", // and {} and {}", + store); + + EXPECT_EQ("42", result); +} + TEST(StringViewTest, ValueType) { static_assert(std::is_same::value, ""); } diff --git a/test/format-dyn-args-test.cc b/test/format-dyn-args-test.cc index da4da7ff..acc5ef78 100644 --- a/test/format-dyn-args-test.cc +++ b/test/format-dyn-args-test.cc @@ -4,84 +4,3 @@ #include #include "gtest-extra.h" - -TEST(FormatDynArgsTest, Basic) { - fmt::dynamic_format_arg_store store; - store.push_back(42); - store.push_back("abc1"); - store.push_back(1.5f); - - std::string result = fmt::vformat("{} and {} and {}", store); - - EXPECT_EQ("42 and abc1 and 1.5", result); -} - -TEST(FormatDynArgsTest, StringsAndRefs) { - // Unfortunately the tests are compiled with old ABI - // So strings use COW. - fmt::dynamic_format_arg_store store; - char str[]{"1234567890"}; - store.push_back(str); - store.push_back(std::cref(str)); - store.push_back(fmt::string_view{str}); - str[0] = 'X'; - - std::string result = fmt::vformat("{} and {} and {}", store); - - EXPECT_EQ("1234567890 and X234567890 and X234567890", result); -} - -struct custom_type { - int i{0}; -}; -FMT_BEGIN_NAMESPACE - -template <> struct formatter { - auto parse(format_parse_context& ctx) const -> decltype(ctx.begin()) { - return ctx.begin(); - } - - template - auto format(const custom_type& p, FormatContext& ctx) -> decltype(format_to( - ctx.out(), std::declval())) { - return format_to(ctx.out(), "cust={}", p.i); - } -}; -FMT_END_NAMESPACE - -TEST(FormatDynArgsTest, CustomFormat) { - fmt::dynamic_format_arg_store store; - custom_type c{}; - store.push_back(c); - ++c.i; - store.push_back(c); - ++c.i; - store.push_back(std::cref(c)); - ++c.i; - - std::string result = fmt::vformat("{} and {} and {}", store); - - EXPECT_EQ("cust=0 and cust=1 and cust=3", result); -} - -TEST(FormatDynArgsTest, NamedArgByRef) { - fmt::dynamic_format_arg_store store; - - // Note: fmt::arg() constructs an object which holds a reference - // to its value. It's not an aggregate, so it doesn't extend the - // reference lifetime. As a result, it's a very bad idea passing temporary - // as a named argument value. Only GCC with optimization level >0 - // complains about this. - // - // A real life usecase is when you have both name and value alive - // guarantee their lifetime and thus don't want them to be copied into - // storages. - int a1_val{42}; - auto a1 = fmt::arg("a1_", a1_val); - store.push_back(std::cref(a1)); - - std::string result = fmt::vformat("{a1_}", // and {} and {}", - store); - - EXPECT_EQ("42", result); -}