Simplify format member detection in FormatVariadic

Summary:
This replaces the format member search, which was quite complicated, with a more
direct approach to detecting whether a class should be formatted using the
format-member method. Instead we use a special type llvm::format_adapter, which
every adapter must inherit from. Then the search can be simply implemented with
the is_base_of type trait.

Aside from the simplification, I like this way more because it makes it more
explicit that you are supposed to use this type only for adapter-like
formattings, and the other approach (format_provider overloads) should be used
as a default (a mistake I made when first trying to use this library).

The only slight change in behaviour here is that now choose the format-adapter
branch even if the format member invocation will fail to compile (e.g. because it is a
non-const member function and we are passing a const adapter), whereas
previously we would have gone on to search for format_providers for the type.
However, I think that is actually a good thing, as it probably means the
programmer did something wrong.

Reviewers: zturner, inglorion

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D27679

llvm-svn: 289795

llvm/docs/ProgrammersManual.rst

@@ -331,16 +331,15 @@ There are two ways to customize the formatting behavior for a type.
   to extend the mechanism for formatting a type that the library already knows how to
   format.  For that, we need something else.
     
-2. Provide a **format adapter** with a non-static format method.
+2. Provide a **format adapter** inheriting from ``llvm::FormatAdapter<T>``.
 
   .. code-block:: c++
   
     namespace anything {
-      struct format_int_custom {
-        int N;
-        explicit format_int_custom(int N) : N(N) {}
-        void format(llvm::raw_ostream &Stream, StringRef Style) {
-          // Do whatever is necessary to format ``N`` into ``Stream``
+      struct format_int_custom : public llvm::FormatAdapter<int> {
+        explicit format_int_custom(int N) : llvm::FormatAdapter<int>(N) {}
+        void format(llvm::raw_ostream &Stream, StringRef Style) override {
+          // Do whatever is necessary to format ``this->Item`` into ``Stream``
         }
       };
     }
@@ -350,9 +349,8 @@ There are two ways to customize the formatting behavior for a type.
       }
     }
     
-  If the search for a specialization of ``format_provider<T>`` for the given type
-  fails, ``formatv`` will subsequently check the argument for an instance method
-  named ``format`` with the signature described above.  If so, it will call the
+  If the type is detected to be derived from ``FormatAdapter<T>``, ``formatv``
+  will call the
   ``format`` method on the argument passing in the specified style.  This allows
   one to provide custom formatting of any type, including one which already has
   a builtin format provider.

llvm/include/llvm/Support/FormatAdapters.h

@@ -17,57 +17,58 @@
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
-template <typename T> class AdapterBase {
+template <typename T> class FormatAdapter : public detail::format_adapter {
 protected:
-  explicit AdapterBase(T &&Item) : Item(Item) {}
+  explicit FormatAdapter(T &&Item) : Item(Item) {}
 
   T Item;
+
   static_assert(!detail::uses_missing_provider<T>::value,
                 "Item does not have a format provider!");
 };
 
 namespace detail {
-template <typename T> class AlignAdapter : public AdapterBase<T> {
+template <typename T> class AlignAdapter final : public FormatAdapter<T> {
   AlignStyle Where;
   size_t Amount;
 
 public:
   AlignAdapter(T &&Item, AlignStyle Where, size_t Amount)
-      : AdapterBase<T>(std::forward<T>(Item)), Where(Where), Amount(Amount) {}
+      : FormatAdapter<T>(std::forward<T>(Item)), Where(Where), Amount(Amount) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Wrapper = detail::build_format_wrapper(std::forward<T>(this->Item));
-    FmtAlign(Wrapper, Where, Amount).format(Stream, Style);
+    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
+    FmtAlign(Adapter, Where, Amount).format(Stream, Style);
   }
 };
 
-template <typename T> class PadAdapter : public AdapterBase<T> {
+template <typename T> class PadAdapter final : public FormatAdapter<T> {
   size_t Left;
   size_t Right;
 
 public:
   PadAdapter(T &&Item, size_t Left, size_t Right)
-      : AdapterBase<T>(std::forward<T>(Item)), Left(Left), Right(Right) {}
+      : FormatAdapter<T>(std::forward<T>(Item)), Left(Left), Right(Right) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Wrapper = detail::build_format_wrapper(std::forward<T>(this->Item));
+    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
     Stream.indent(Left);
-    Wrapper.format(Stream, Style);
+    Adapter.format(Stream, Style);
     Stream.indent(Right);
   }
 };
 
-template <typename T> class RepeatAdapter : public AdapterBase<T> {
+template <typename T> class RepeatAdapter final : public FormatAdapter<T> {
   size_t Count;
 
 public:
   RepeatAdapter(T &&Item, size_t Count)
-      : AdapterBase<T>(std::forward<T>(Item)), Count(Count) {}
+      : FormatAdapter<T>(std::forward<T>(Item)), Count(Count) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Wrapper = detail::build_format_wrapper(std::forward<T>(this->Item));
+    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
     for (size_t I = 0; I < Count; ++I) {
-      Wrapper.format(Stream, Style);
+      Adapter.format(Stream, Style);
     }
   }
 };

llvm/include/llvm/Support/FormatCommon.h

@@ -18,12 +18,12 @@ namespace llvm {
 enum class AlignStyle { Left, Center, Right };
 
 struct FmtAlign {
-  detail::format_wrapper &Wrapper;
+  detail::format_adapter &Adapter;
   AlignStyle Where;
   size_t Amount;
 
-  FmtAlign(detail::format_wrapper &Wrapper, AlignStyle Where, size_t Amount)
-      : Wrapper(Wrapper), Where(Where), Amount(Amount) {}
+  FmtAlign(detail::format_adapter &Adapter, AlignStyle Where, size_t Amount)
+      : Adapter(Adapter), Where(Where), Amount(Amount) {}
 
   void format(raw_ostream &S, StringRef Options) {
     // If we don't need to align, we can format straight into the underlying
@@ -32,13 +32,13 @@ struct FmtAlign {
     // TODO: Make the format method return the number of bytes written, that
     // way we can also skip the intermediate stream for left-aligned output.
     if (Amount == 0) {
-      Wrapper.format(S, Options);
+      Adapter.format(S, Options);
       return;
     }
     SmallString<64> Item;
     raw_svector_ostream Stream(Item);
 
-    Wrapper.format(Stream, Options);
+    Adapter.format(Stream, Options);
     if (Amount <= Item.size()) {
       S << Item;
       return;

llvm/include/llvm/Support/FormatProviders.h

@@ -394,16 +394,16 @@ public:
     auto Begin = V.begin();
     auto End = V.end();
     if (Begin != End) {
-      auto Wrapper =
-          detail::build_format_wrapper(std::forward<reference>(*Begin));
-      Wrapper.format(Stream, ArgStyle);
+      auto Adapter =
+          detail::build_format_adapter(std::forward<reference>(*Begin));
+      Adapter.format(Stream, ArgStyle);
       ++Begin;
     }
     while (Begin != End) {
       Stream << Sep;
-      auto Wrapper =
-          detail::build_format_wrapper(std::forward<reference>(*Begin));
-      Wrapper.format(Stream, ArgStyle);
+      auto Adapter =
+          detail::build_format_adapter(std::forward<reference>(*Begin));
+      Adapter.format(Stream, ArgStyle);
       ++Begin;
     }
   }

llvm/include/llvm/Support/FormatVariadic.h

@@ -71,15 +71,15 @@ protected:
   // parameters in a template class that derives from a non-template superclass.
   // Essentially, we are converting a std::tuple<Derived<Ts...>> to a
   // std::vector<Base*>.
-  struct create_wrappers {
+  struct create_adapters {
     template <typename... Ts>
-    std::vector<detail::format_wrapper *> operator()(Ts &... Items) {
-      return std::vector<detail::format_wrapper *>{&Items...};
+    std::vector<detail::format_adapter *> operator()(Ts &... Items) {
+      return std::vector<detail::format_adapter *>{&Items...};
     }
   };
 
   StringRef Fmt;
-  std::vector<detail::format_wrapper *> Wrappers;
+  std::vector<detail::format_adapter *> Adapters;
   std::vector<ReplacementItem> Replacements;
 
   static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
@@ -91,7 +91,7 @@ protected:
 public:
   formatv_object_base(StringRef Fmt, std::size_t ParamCount)
       : Fmt(Fmt), Replacements(parseFormatString(Fmt)) {
-    Wrappers.reserve(ParamCount);
+    Adapters.reserve(ParamCount);
   }
 
   void format(raw_ostream &S) const {
@@ -102,12 +102,12 @@ public:
         S << R.Spec;
         continue;
       }
-      if (R.Index >= Wrappers.size()) {
+      if (R.Index >= Adapters.size()) {
         S << R.Spec;
         continue;
       }
 
-      auto W = Wrappers[R.Index];
+      auto W = Adapters[R.Index];
 
       FmtAlign Align(*W, R.Where, R.Align);
       Align.format(S, R.Options);
@@ -138,7 +138,7 @@ public:
 };
 
 template <typename Tuple> class formatv_object : public formatv_object_base {
-  // Storage for the parameter wrappers.  Since the base class erases the type
+  // Storage for the parameter adapters.  Since the base class erases the type
   // of the parameters, we have to own the storage for the parameters here, and
   // have the base class store type-erased pointers into this tuple.
   Tuple Parameters;
@@ -147,7 +147,7 @@ public:
   formatv_object(StringRef Fmt, Tuple &&Params)
       : formatv_object_base(Fmt, std::tuple_size<Tuple>::value),
         Parameters(std::move(Params)) {
-    Wrappers = apply_tuple(create_wrappers(), Parameters);
+    Adapters = apply_tuple(create_adapters(), Parameters);
   }
 };
 
@@ -234,12 +234,12 @@ public:
 //
 template <typename... Ts>
 inline auto formatv(const char *Fmt, Ts &&... Vals) -> formatv_object<decltype(
-    std::make_tuple(detail::build_format_wrapper(std::forward<Ts>(Vals))...))> {
+    std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
   using ParamTuple = decltype(
-      std::make_tuple(detail::build_format_wrapper(std::forward<Ts>(Vals))...));
+      std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...));
   return formatv_object<ParamTuple>(
       Fmt,
-      std::make_tuple(detail::build_format_wrapper(std::forward<Ts>(Vals))...));
+      std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...));
 }
 
 } // end namespace llvm

llvm/include/llvm/Support/FormatVariadicDetails.h

@@ -19,83 +19,26 @@ namespace llvm {
 template <typename T, typename Enable = void> struct format_provider {};
 
 namespace detail {
-
-class format_wrapper {
+class format_adapter {
 protected:
-  virtual ~format_wrapper() {}
+  virtual ~format_adapter() {}
 
 public:
-  virtual void format(llvm::raw_ostream &S, StringRef Options) = 0;
+  virtual void format(raw_ostream &S, StringRef Options) = 0;
 };
 
-template <typename T> class member_format_wrapper : public format_wrapper {
+template <typename T> class provider_format_adapter : public format_adapter {
   T Item;
 
 public:
-  explicit member_format_wrapper(T &&Item) : Item(Item) {}
-
-  void format(llvm::raw_ostream &S, StringRef Options) override {
-    Item.format(S, Options);
-  }
-};
-
-template <typename T> class provider_format_wrapper : public format_wrapper {
-  T Item;
-
-public:
-  explicit provider_format_wrapper(T &&Item) : Item(Item) {}
+  explicit provider_format_adapter(T &&Item) : Item(Item) {}
 
   void format(llvm::raw_ostream &S, StringRef Options) override {
     format_provider<typename std::decay<T>::type>::format(Item, S, Options);
   }
 };
 
-template <typename T> class missing_format_wrapper;
-
-// Test if T is a class that contains a member function with the signature:
-//
-// void format(raw_ostream &, StringRef);
-//
-// It is assumed T is a non-reference type.
-template <class T, class Enable = void> class has_FormatMember {
-public:
-  static bool const value = false;
-};
-
-template <class T>
-class has_FormatMember<T,
-                       typename std::enable_if<std::is_class<T>::value &&
-                                               std::is_const<T>::value>::type> {
-  using CleanT = typename std::remove_volatile<T>::type;
-  using Signature_format = void (CleanT::*)(llvm::raw_ostream &S,
-                                            StringRef Options) const;
-
-  template <typename U>
-  static char test2(SameType<Signature_format, &U::format> *);
-
-  template <typename U> static double test2(...);
-
-public:
-  static bool const value = (sizeof(test2<CleanT>(nullptr)) == 1);
-};
-
-template <class T>
-class has_FormatMember<
-    T, typename std::enable_if<std::is_class<T>::value &&
-                               !std::is_const<T>::value>::type> {
-  using CleanT = typename std::remove_cv<T>::type;
-  using Signature_format = void (CleanT::*)(llvm::raw_ostream &S,
-                                            StringRef Options);
-
-  template <typename U>
-  static char test2(SameType<Signature_format, &U::format> *);
-
-  template <typename U> static double test2(...);
-
-public:
-  static bool const value =
-      (sizeof(test2<CleanT>(nullptr)) == 1) || has_FormatMember<const T>::value;
-};
+template <typename T> class missing_format_adapter;
 
 // Test if format_provider<T> is defined on T and contains a member function
 // with the signature:
@@ -122,7 +65,8 @@ template <typename T>
 struct uses_format_member
     : public std::integral_constant<
           bool,
-          has_FormatMember<typename std::remove_reference<T>::type>::value> {};
+          std::is_base_of<format_adapter,
+                          typename std::remove_reference<T>::type>::value> {};
 
 // Simple template that decides whether a type T should use the format_provider
 // based format() invocation.  The member function takes priority, so this test
@@ -144,24 +88,23 @@ struct uses_missing_provider
                                         !uses_format_provider<T>::value> {};
 
 template <typename T>
-typename std::enable_if<uses_format_member<T>::value,
-                        member_format_wrapper<T>>::type
-build_format_wrapper(T &&Item) {
-  return member_format_wrapper<T>(std::forward<T>(Item));
+typename std::enable_if<uses_format_member<T>::value, T>::type
+build_format_adapter(T &&Item) {
+  return std::forward<T>(Item);
 }
 
 template <typename T>
 typename std::enable_if<uses_format_provider<T>::value,
-                        provider_format_wrapper<T>>::type
-build_format_wrapper(T &&Item) {
-  return provider_format_wrapper<T>(std::forward<T>(Item));
+                        provider_format_adapter<T>>::type
+build_format_adapter(T &&Item) {
+  return provider_format_adapter<T>(std::forward<T>(Item));
 }
 
 template <typename T>
 typename std::enable_if<uses_missing_provider<T>::value,
-                        missing_format_wrapper<T>>::type
-build_format_wrapper(T &&Item) {
-  return missing_format_wrapper<T>();
+                        missing_format_adapter<T>>::type
+build_format_adapter(T &&Item) {
+  return missing_format_adapter<T>();
 }
 }
 }

llvm/unittests/Support/FormatVariadicTest.cpp

@@ -13,33 +13,26 @@
 
 using namespace llvm;
 
-// Compile-time tests for the uses_format_member template
+// Compile-time tests templates in the detail namespace.
 namespace {
-struct ConstFormat {
-  void format(raw_ostream &OS, StringRef Opt) const { OS << "ConstFormat"; }
-};
-
-struct Format {
-  void format(raw_ostream &OS, StringRef Opt) { OS << "Format"; }
+struct Format : public FormatAdapter<int> {
+  Format(int N) : FormatAdapter<int>(std::move(N)) {}
+  void format(raw_ostream &OS, StringRef Opt) override { OS << "Format"; }
 };
 
 using detail::uses_format_member;
+using detail::uses_missing_provider;
 
 static_assert(uses_format_member<Format>::value, "");
 static_assert(uses_format_member<Format &>::value, "");
 static_assert(uses_format_member<Format &&>::value, "");
-static_assert(!uses_format_member<const Format>::value, "");
-static_assert(!uses_format_member<const Format &>::value, "");
-static_assert(!uses_format_member<const volatile Format>::value, "");
-static_assert(!uses_format_member<const volatile Format &>::value, "");
+static_assert(uses_format_member<const Format>::value, "");
+static_assert(uses_format_member<const Format &>::value, "");
+static_assert(uses_format_member<const volatile Format>::value, "");
+static_assert(uses_format_member<const volatile Format &>::value, "");
 
-static_assert(uses_format_member<ConstFormat>::value, "");
-static_assert(uses_format_member<ConstFormat &>::value, "");
-static_assert(uses_format_member<ConstFormat &&>::value, "");
-static_assert(uses_format_member<const ConstFormat>::value, "");
-static_assert(uses_format_member<const ConstFormat &>::value, "");
-static_assert(uses_format_member<const volatile ConstFormat>::value, "");
-static_assert(uses_format_member<const volatile ConstFormat &>::value, "");
+struct NoFormat {};
+static_assert(uses_missing_provider<NoFormat>::value, "");
 }
 
 TEST(FormatVariadicTest, EmptyFormatString) {
@@ -535,12 +528,10 @@ TEST(FormatVariadicTest, Range) {
 }
 
 TEST(FormatVariadicTest, Adapter) {
-  class Negative {
-    int N;
-
+  class Negative : public FormatAdapter<int> {
   public:
-    explicit Negative(int N) : N(N) {}
-    void format(raw_ostream &S, StringRef Options) const { S << -N; }
+    explicit Negative(int N) : FormatAdapter<int>(std::move(N)) {}
+    void format(raw_ostream &S, StringRef Options) override { S << -Item; }
   };
 
   EXPECT_EQ("-7", formatv("{0}", Negative(7)).str());
@@ -566,25 +557,14 @@ TEST(FormatVariadicTest, ImplicitConversions) {
   EXPECT_EQ("1 2", S2);
 }
 
-TEST(FormatVariadicTest, FormatMember) {
-  EXPECT_EQ("Format", formatv("{0}", Format()).str());
+TEST(FormatVariadicTest, FormatAdapter) {
+  EXPECT_EQ("Format", formatv("{0}", Format(1)).str());
 
-  Format var;
+  Format var(1);
   EXPECT_EQ("Format", formatv("{0}", var).str());
   EXPECT_EQ("Format", formatv("{0}", std::move(var)).str());
 
   // Not supposed to compile
-  // const Format cvar{};
+  // const Format cvar(1);
   // EXPECT_EQ("Format", formatv("{0}", cvar).str());
 }
-
-TEST(FormatVariadicTest, FormatMemberConst) {
-  EXPECT_EQ("ConstFormat", formatv("{0}", ConstFormat()).str());
-
-  ConstFormat var;
-  EXPECT_EQ("ConstFormat", formatv("{0}", var).str());
-  EXPECT_EQ("ConstFormat", formatv("{0}", std::move(var)).str());
-
-  const ConstFormat cvar{};
-  EXPECT_EQ("ConstFormat", formatv("{0}", cvar).str());
-}