Minor tweaks for dynamic_format_arg_store

include/fmt/core.h

@@ -1205,6 +1205,62 @@ template <bool IS_PACKED, typename Context, typename T,
 inline basic_format_arg<Context> make_arg(const T& value) {
   return make_arg<Context>(value);
 }
+
+template <typename T, typename Char> struct is_string_view : std::false_type {};
+
+template <typename Char>
+struct is_string_view<basic_string_view<Char>, Char> : std::true_type {};
+
+template <typename Char>
+struct is_string_view<std_string_view<Char>, Char> : std::true_type {};
+
+template <typename T> struct is_reference_wrapper : std::false_type {};
+
+template <typename T>
+struct is_reference_wrapper<std::reference_wrapper<T>> : 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 <typename = void> struct storage_node_base {
+    using owning_ptr = std::unique_ptr<storage_node_base<>>;
+    virtual ~storage_node_base() = default;
+    owning_ptr next;
+  };
+
+  template <typename T> struct storage_node : storage_node_base<> {
+    T value;
+
+    template <typename Arg>
+    FMT_CONSTEXPR storage_node(const Arg& arg, owning_ptr&& next) : value(arg) {
+      this->next = std::move(next);  // Must be initialised after value.
+    }
+
+    template <typename Char>
+    FMT_CONSTEXPR storage_node(const basic_string_view<Char>& 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 <typename T, typename Arg> const T& push(const Arg& arg) {
+    auto node = new storage_node<T>(arg, std::move(head_));
+    head_.reset(node);
+    return node->value;
+  }
+};
 }  // namespace internal
 
 // Formatting context.
@@ -1314,7 +1370,108 @@ inline format_arg_store<Context, Args...> make_format_args(
   return {args...};
 }
 
-template <typename Context> 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 <typename Context>
+class dynamic_format_arg_store
+#if FMT_GCC_VERSION < 409
+    // Workaround a GCC template argument substitution bug.
+    : public basic_format_args<Context>
+#endif
+{
+ private:
+  using char_type = typename Context::char_type;
+
+  template <typename T> struct need_dyn_copy {
+    static constexpr internal::type mapped_type =
+        internal::mapped_type_constant<T, Context>::value;
+
+    using type = std::integral_constant<
+        bool, !(internal::is_reference_wrapper<T>::value ||
+                internal::is_string_view<T, char_type>::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 <typename T>
+  using stored_type = conditional_t<internal::is_string<T>::value,
+                                    std::basic_string<char_type>, T>;
+
+  // Storage of basic_format_arg must be contiguous.
+  std::vector<basic_format_arg<Context>> 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<Context>;
+
+  unsigned long long get_types() const {
+    return internal::is_unpacked_bit | data_.size();
+  }
+
+  template <typename T> void emplace_arg(const T& arg) {
+    data_.emplace_back(internal::make_arg<Context>(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<fmt::format_context> store;
+      store.push_back(42);
+      store.push_back("abc");
+      store.push_back(1.5f);
+      std::string result = fmt::vformat("{} and {} and {}", store);
+    \endrst
+  */
+  template <typename T> void push_back(const T& arg) {
+    static_assert(
+        !std::is_base_of<internal::named_arg_base<char_type>, T>::value,
+        "named arguments are not supported yet");
+    if (internal::const_check(need_dyn_copy<T>::type::value))
+      emplace_arg(storage_.push<stored_type<T>>(arg));
+    else
+      emplace_arg(arg);
+  }
+
+  /**
+    Adds a reference to the argument into the dynamic store for later passing to
+    a formating function.
+  */
+  template <typename T> void push_back(std::reference_wrapper<T> arg) {
+    static_assert(
+        need_dyn_copy<T>::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<Args...>(format_str, args...));
 #endif
 }
-
-namespace internal {
-
-template <typename T, typename Char> struct is_string_view : std::false_type {};
-
-template <typename Char>
-struct is_string_view<basic_string_view<Char>, Char> : std::true_type {};
-
-template <typename Char>
-struct is_string_view<std_string_view<Char>, Char> : std::true_type {};
-
-template <typename T> struct is_reference_wrapper : std::false_type {};
-
-template <typename T>
-struct is_reference_wrapper<std::reference_wrapper<T>> : 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 <typename = void> struct storage_node_base {
-    using owning_ptr = std::unique_ptr<storage_node_base<>>;
-    virtual ~storage_node_base() = default;
-    owning_ptr next;
-  };
-
-  template <typename T> struct storage_node : storage_node_base<> {
-    T value;
-    template <typename Arg>
-    FMT_CONSTEXPR storage_node(const Arg& arg, owning_ptr&& next) : value{arg} {
-      // Must be initialised after value_
-      this->next = std::move(next);
-    }
-
-    template <typename Char>
-    FMT_CONSTEXPR storage_node(const basic_string_view<Char>& 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 <typename T, typename Arg> const T& push(const Arg& arg) {
-    auto node = new storage_node<T>{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 <typename Context>
-class dynamic_format_arg_store
-#if FMT_GCC_VERSION < 409
-    // Workaround a GCC template argument substitution bug.
-    : public basic_format_args<Context>
-#endif
-{
- private:
-  using char_type = typename Context::char_type;
-
-  template <typename T> struct need_dyn_copy {
-    static constexpr internal::type mapped_type =
-        internal::mapped_type_constant<T, Context>::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<T>::value ||
-                internal::is_string_view<T, char_type>::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 <typename T>
-  using stored_type = conditional_t<internal::is_string<T>::value,
-                                    std::basic_string<char_type>, T>;
-
-  // Storage of basic_format_arg must be contiguous
-  // Required by basic_format_args::args_ which is just a pointer.
-  std::vector<basic_format_arg<Context>> 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<Context>;
-
-  unsigned long long get_types() const {
-    return internal::is_unpacked_bit | data_.size();
-  }
-
-  template <typename T> void emplace_arg(const T& arg) {
-    data_.emplace_back(internal::make_arg<Context>(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/core.h>
-      fmt::dynamic_format_arg_store<fmt::format_context> store;
-      store.push_back(42);
-      store.push_back("abc1");
-      store.push_back(1.5f);
-      std::string result = fmt::vformat("{} and {} and {}", store);
-    \endrst
-  */
-  template <typename T> void push_back(const T& arg) {
-    static_assert(
-        !std::is_base_of<internal::named_arg_base<char_type>, T>::value,
-        "Named arguments are not supported yet");
-    if (internal::const_check(need_dyn_copy<T>::type::value))
-      emplace_arg(storage_.push<stored_type<T>>(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 <typename T> void push_back(std::reference_wrapper<T> arg) {
-    static_assert(
-        need_dyn_copy<T>::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_

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 ()

test/core-test.cc

@@ -15,9 +15,8 @@
 #include <string>
 #include <type_traits>
 
-#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<fmt::format_context> 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<fmt::format_context> 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<custom_type> {
+  auto parse(format_parse_context& ctx) const -> decltype(ctx.begin()) {
+    return ctx.begin();
+  }
+
+  template <typename FormatContext>
+  auto format(const custom_type& p, FormatContext& ctx) -> decltype(format_to(
+      ctx.out(), std::declval<typename FormatContext::char_type const*>())) {
+    return format_to(ctx.out(), "cust={}", p.i);
+  }
+};
+FMT_END_NAMESPACE
+
+TEST(FormatDynArgsTest, CustomFormat) {
+  fmt::dynamic_format_arg_store<fmt::format_context> 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<fmt::format_context> 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<string_view::value_type, char>::value, "");
 }

test/format-dyn-args-test.cc

@@ -4,84 +4,3 @@
 #include <fmt/core.h>
 
 #include "gtest-extra.h"
-
-TEST(FormatDynArgsTest, Basic) {
-  fmt::dynamic_format_arg_store<fmt::format_context> 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<fmt::format_context> 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<custom_type> {
-  auto parse(format_parse_context& ctx) const -> decltype(ctx.begin()) {
-    return ctx.begin();
-  }
-
-  template <typename FormatContext>
-  auto format(const custom_type& p, FormatContext& ctx) -> decltype(format_to(
-      ctx.out(), std::declval<typename FormatContext::char_type const*>())) {
-    return format_to(ctx.out(), "cust={}", p.i);
-  }
-};
-FMT_END_NAMESPACE
-
-TEST(FormatDynArgsTest, CustomFormat) {
-  fmt::dynamic_format_arg_store<fmt::format_context> 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<fmt::format_context> 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);
-}