[Format] Simplify some of the formatter template glue

[zturner]

Summary

The original idea here was that we would take the parameter pack and store it as both a tuple and a vector. To put things in a vector the items had to share a common base, so I made this format_adapter_base. Then there were 2 implementations. One implementation that would be used for types where the format method would invoke a member function on the type itself, and another where a global formatter would be used.

But we can make this simpler though. The only thing we need to store is just the value itself, and we can defer the knowledge of what kind of formatter it uses until we actually go to format it, and we can have the final selection just delegate to a global template function that is properly specialized. This has the advantage of making the template glue much simpler, but also allowing us a convenient syntax to define new formatters via a macro. A few existing formatters are converted to this new syntax as an example. The rest can be done in a followup, for now I just want to see if the approach is ok.

Eventually I'd like to get the point where the adapters are composable, so you can say like align(pad(Value, 7), Left, 3) but part of that means simplifying the underlying glue so that there's fewer moving parts.

[dblaikie]

llvm/include/llvm/Support/FormatProviders.h

28–33

Not sure this benefits much from being a macro, does it?

Calls to these functions look like they're ADL compatible, so there's no need to put them in the llvm namespace - they can go in whatever namespace the Type is in, probably? (doesn't matter too much in the LLVM project - we don't need to protect the llvm namespace, etc, but is perhaps a more generic extensibility mechanism than users having to write function definitions in the library's namespace)

394–396

Skip braces on one-line if?

395

Usually assert(false) should be llvm_unreachable, but branch-to-unreachable isn't ideal either, so perhaps:

if (!Style.empty()) {
  bool Success = Style.getAsInteger(10, N);
  (void)Success;
  assert(Success && "Style is not a valid integer");
}

Yeah, I know it's a couple of lines longer, which is awkward too :/

I guess if getAsInteger returned Expected<size_t> might be a bit tidier:

if (!Style.empty())
  N = cantFail(Style.getAsInteger<size_t>(10));

404

If you can avoid turning a Twine into a std::string it could save some memory allocation/copying/processing.

But, yeah, supporting the length limiting support of the string formatter above would be a bit of work interacting with Twine's print function. (maybe Twine could use a template that takes a functor and is called back on the various kinds of elements it has) - something for another day I guess.

llvm/include/llvm/Support/FormatVariadicDetails.h

79–83

Does this support (for format members) pull its weight? Compared to all format extensibility going through a single ADL signature? (the definition of a non-member format function where the member format is written would be almost identical, if defined as an inline friend definition inside the class definition (you have to mention the type name in the signature and name that variable rather than using 'this'))

[labath]

I am not sure I can bring much to this, so I'll take a back seat.

llvm/include/llvm/Support/FormatProviders.h

28–33

I don't know about the technical feasibility of this, but I did feel awkward every time I had to write "namespace llvm" to write a lldb pretty-printer.

llvm/include/llvm/Support/FormatAdapters.h

template <typename T> class AlignAdapter final : public FormatAdapter<T> {
   char Fill;
 
 public:
   AlignAdapter(T &&Item, AlignStyle Where, size_t Amount, char Fill)
       : FormatAdapter<T>(std::forward<T>(Item)), Where(Where), Amount(Amount),
         Fill(Fill) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
-    FmtAlign(Adapter, Where, Amount, Fill).format(Stream, Style);
+    value_holder<T> Holder(std::forward<T>(this->Item));
+    FmtAlign(Holder, Where, Amount, Fill).format(Stream, Style);
   }
 };
 
 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)
       : FormatAdapter<T>(std::forward<T>(Item)), Left(Left), Right(Right) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
     Stream.indent(Left);
-    Adapter.format(Stream, Style);
+    format_one_item(std::forward<T>(this->Item), Stream, Style);
     Stream.indent(Right);
   }
 };
 
 template <typename T> class RepeatAdapter final : public FormatAdapter<T> {
   size_t Count;
 
 public:
   RepeatAdapter(T &&Item, size_t Count)
       : FormatAdapter<T>(std::forward<T>(Item)), Count(Count) {}
 
   void format(llvm::raw_ostream &Stream, StringRef Style) {
-    auto Adapter = detail::build_format_adapter(std::forward<T>(this->Item));
     for (size_t I = 0; I < Count; ++I) {
-      Adapter.format(Stream, Style);
+      format_one_item(std::forward<T>(this->Item), Stream, Style);
     }
   }
 };
 }
 
 template <typename T>
 detail::AlignAdapter<T> fmt_align(T &&Item, AlignStyle Where, size_t Amount,
                                   char Fill = ' ') {

llvm/include/llvm/Support/FormatProviders.h

 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/FormatVariadicDetails.h"
 #include "llvm/Support/NativeFormatting.h"
 
 #include <type_traits>
 #include <vector>
 
+#define FORMATTER(Type)                                                        \
+  namespace llvm {                                                             \
+  void format_one_item(Type &&Item, raw_ostream &S, StringRef Options);        \
+  }                                                                            \
+  inline void llvm::format_one_item(Type &&Value, raw_ostream &Stream,         \
+                                    StringRef Style)
+
+#define FORMAT_IF(Condition)                                                   \
+  namespace llvm {                                                             \
+  template <typename T>                                                        \
+  typename std::enable_if<Condition::value>::type                              \
+  format_one_item(T &&Item, raw_ostream &S, StringRef Options);                \
+  }                                                                            \
+  template <typename T>                                                        \
+  typename std::enable_if<Condition::value>::type llvm::format_one_item(       \
+      T &&Value, llvm::raw_ostream &Stream, llvm::StringRef Style)
+
 namespace llvm {
 namespace detail {
+
 template <typename T>
 struct use_integral_formatter
     : public std::integral_constant<
           bool, is_one_of<T, uint8_t, int16_t, uint16_t, int32_t, uint32_t,
                           int64_t, uint64_t, int, unsigned, long, unsigned long,
                           long long, unsigned long long>::value> {};
 
 template <typename T>
public:
   static void format(const T &V, llvm::raw_ostream &Stream, StringRef Style) {
     HexPrintStyle HS = HexPrintStyle::PrefixUpper;
     consumeHexStyle(Style, HS);
     size_t Digits = consumeNumHexDigits(Style, HS, sizeof(void *) * 2);
     write_hex(Stream, reinterpret_cast<std::uintptr_t>(V), HS, Digits);
   }
 };
 
-/// Implementation of format_provider<T> for c-style strings and string
-/// objects such as std::string and llvm::StringRef.
-///
-/// The options string of a string type has the grammar:
-///
-///   string_options :: [length]
-///
-/// where `length` is an optional integer specifying the maximum number of
-/// characters in the string to print.  If `length` is omitted, the string is
-/// printed up to the null terminator.
-
-template <typename T>
-struct format_provider<
-    T, typename std::enable_if<detail::use_string_formatter<T>::value>::type> {
-  static void format(const T &V, llvm::raw_ostream &Stream, StringRef Style) {
-    size_t N = StringRef::npos;
-    if (!Style.empty() && Style.getAsInteger(10, N)) {
-      assert(false && "Style is not a valid integer");
-    }
-    llvm::StringRef S = V;
-    Stream << S.substr(0, N);
-  }
-};
-
-/// Implementation of format_provider<T> for llvm::Twine.
-///
-/// This follows the same rules as the string formatter.
-
-template <> struct format_provider<Twine> {
-  static void format(const Twine &V, llvm::raw_ostream &Stream,
-                     StringRef Style) {
-    format_provider<std::string>::format(V.str(), Stream, Style);
-  }
-};
-
-/// Implementation of format_provider<T> for characters.
-///
-/// The options string of a character type has the grammar:
-///
-///   char_options :: (empty) | [integer_options]
-///
-/// If `char_options` is empty, the character is displayed as an ASCII
-/// character.  Otherwise, it is treated as an integer options string.
-///
-template <typename T>
-struct format_provider<
-    T, typename std::enable_if<detail::use_char_formatter<T>::value>::type> {
-  static void format(const char &V, llvm::raw_ostream &Stream,
-                     StringRef Style) {
-    if (Style.empty())
-      Stream << V;
-    else {
-      int X = static_cast<int>(V);
-      format_provider<int>::format(X, Stream, Style);
-    }
-  }
-};
-
-/// Implementation of format_provider<T> for type `bool`
-///
-/// The options string of a boolean type has the grammar:
-///
-///   bool_options :: "" | "Y" | "y" | "D" | "d" | "T" | "t"
-///
-///   ==================================
-///   |    C    |     Meaning          |
-///   ==================================
-///   |    Y    |       YES / NO       |
-///   |    y    |       yes / no       |
-///   |  D / d  |    Integer 0 or 1    |
-///   |    T    |     TRUE / FALSE     |
-///   |    t    |     true / false     |
-///   | (empty) |   Equivalent to 't'  |
-///   ==================================
-template <> struct format_provider<bool> {
-  static void format(const bool &B, llvm::raw_ostream &Stream,
-                     StringRef Style) {
-    Stream << StringSwitch<const char *>(Style)
-                  .Case("Y", B ? "YES" : "NO")
-                  .Case("y", B ? "yes" : "no")
-                  .CaseLower("D", B ? "1" : "0")
-                  .Case("T", B ? "TRUE" : "FALSE")
-                  .Cases("t", "", B ? "true" : "false")
-                  .Default(B ? "1" : "0");
-  }
-};
-
 /// Implementation of format_provider<T> for floating point types.
 ///
 /// The options string of a floating point type has the format:
 ///
 ///   float_options   :: [style][precision]
 ///   style           :: <see table below>
 ///   precision       :: <non-negative integer> 0-99
 ///
static void format(const T &V, llvm::raw_ostream &Stream, StringRef Style) {
     Optional<size_t> Precision = parseNumericPrecision(Style);
     if (!Precision.hasValue())
       Precision = getDefaultPrecision(S);
 
     write_double(Stream, static_cast<double>(V), S, Precision);
   }
 };
 
-namespace detail {
-template <typename IterT>
-using IterValue = typename std::iterator_traits<IterT>::value_type;
-
-template <typename IterT>
-struct range_item_has_provider
-    : public std::integral_constant<
-          bool, !uses_missing_provider<IterValue<IterT>>::value> {};
-}
-
 /// Implementation of format_provider<T> for ranges.
 ///
 /// This will print an arbitrary range as a delimited sequence of items.
 ///
 /// The options string of a range type has the grammar:
 ///
 ///   range_style       ::= [separator] [element_style]
 ///   separator         ::= "$" delimeted_expr
template <typename IterT> class format_provider<llvm::iterator_range<IterT>> {
   static std::pair<StringRef, StringRef> parseOptions(StringRef Style) {
     StringRef Sep = consumeOneOption(Style, '$', ", ");
     StringRef Args = consumeOneOption(Style, '@', "");
     assert(Style.empty() && "Unexpected text in range option string!");
     return std::make_pair(Sep, Args);
   }
 
 public:
-  static_assert(detail::range_item_has_provider<IterT>::value,
-                "Range value_type does not have a format provider!");
   static void format(const llvm::iterator_range<IterT> &V,
                      llvm::raw_ostream &Stream, StringRef Style) {
     StringRef Sep;
     StringRef ArgStyle;
     std::tie(Sep, ArgStyle) = parseOptions(Style);
     auto Begin = V.begin();
     auto End = V.end();
     if (Begin != End) {
-      auto Adapter =
-          detail::build_format_adapter(std::forward<reference>(*Begin));
-      Adapter.format(Stream, ArgStyle);
+      format_one_item(*Begin, Stream, ArgStyle);
       ++Begin;
     }
     while (Begin != End) {
       Stream << Sep;
-      auto Adapter =
-          detail::build_format_adapter(std::forward<reference>(*Begin));
-      Adapter.format(Stream, ArgStyle);
+      format_one_item(*Begin, Stream, ArgStyle);
       ++Begin;
     }
   }
 };
 }
 
+/// Implementation of format_provider<T> for type `bool`
+///
+/// The options string of a boolean type has the grammar:
+///
+///   bool_options :: "" | "Y" | "y" | "D" | "d" | "T" | "t"
+///
+///   ==================================
+///   |    C    |     Meaning          |
+///   ==================================
+///   |    Y    |       YES / NO       |
+///   |    y    |       yes / no       |
+///   |  D / d  |    Integer 0 or 1    |
+///   |    T    |     TRUE / FALSE     |
+///   |    t    |     true / false     |
+///   | (empty) |   Equivalent to 't'  |
+///   ==================================
+FORMATTER(bool) {
+  Stream << StringSwitch<const char *>(Style)
+                .Case("Y", Value ? "YES" : "NO")
+                .Case("y", Value ? "yes" : "no")
+                .CaseLower("D", Value ? "1" : "0")
+                .Case("T", Value ? "TRUE" : "FALSE")
+                .Cases("t", "", Value ? "true" : "false")
+                .Default(Value ? "1" : "0");
+}
+
+/// Implementation of format_provider<T> for characters.
+///
+/// The options string of a character type has the grammar:
+///
+///   char_options :: (empty) | [integer_options]
+///
+/// If `char_options` is empty, the character is displayed as an ASCII
+/// character.  Otherwise, it is treated as an integer options string.
+///
+FORMATTER(char) {
+  if (Style.empty())
+    Stream << Value;
+  else {
+    int X = static_cast<int>(Value);
+    format_provider<int>::format(X, Stream, Style);
+  }
+}
+
+/// Implementation of format_provider<T> for c-style strings and string
+/// objects such as std::string and llvm::StringRef.
+///
+/// The options string of a string type has the grammar:
+///
+///   string_options :: [length]
+///
+/// where `length` is an optional integer specifying the maximum number of
+/// characters in the string to print.  If `length` is omitted, the string is
+/// printed up to the null terminator.
+FORMAT_IF(llvm::detail::use_string_formatter<T>) {
+  size_t N = StringRef::npos;
+  if (!Style.empty() && Style.getAsInteger(10, N)) {
+    assert(false && "Style is not a valid integer");
+  }
+  llvm::StringRef S = Value;
+  Stream << S.substr(0, N);
+}
+
+/// Implementation of format_provider<T> for llvm::Twine.
+///
+/// This follows the same rules as the string formatter.
+FORMATTER(llvm::Twine) { format_one_item(Value.str(), Stream, Style); };
+
 #endif

llvm/include/llvm/Support/FormatVariadic.h

template <unsigned N> SmallString<N> sstr() const {
     return Result;
   }
 
   template <unsigned N> operator SmallString<N>() const { return sstr<N>(); }
 
   operator std::string() const { return str(); }
 };
 
-template <typename Tuple> class formatv_object : public formatv_object_base {
+template <typename... Ts> class formatv_object : public formatv_object_base {
+  using Tuple = std::tuple<detail::value_holder<Ts>...>;
   // 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;
 
 public:
-  formatv_object(StringRef Fmt, Tuple &&Params)
-      : formatv_object_base(Fmt, std::tuple_size<Tuple>::value),
-        Parameters(std::move(Params)) {
+  formatv_object(StringRef Fmt, Ts &&... Params)
+      : formatv_object_base(Fmt, sizeof...(Params)),
+        Parameters(detail::value_holder<Ts>(std::forward<Ts>(Params))...) {
     Adapters = apply_tuple(create_adapters(), Parameters);
   }
 };
 
 // \brief Format text given a format string and replacement parameters.
 //
 // ===General Description===
 //

 // ===Invalid Format String Handling===
 //
 // In the case of a format string which does not match the grammar described
 // above, the output is undefined.  With asserts enabled, LLVM will trigger an
 // assertion.  Otherwise, it will try to do something reasonable, but in general
 // the details of what that is are undefined.
 //
 template <typename... Ts>
-inline auto formatv(const char *Fmt, Ts &&... Vals) -> formatv_object<decltype(
-    std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
-  using ParamTuple = decltype(
-      std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...));
-  return formatv_object<ParamTuple>(
-      Fmt,
-      std::make_tuple(detail::build_format_adapter(std::forward<Ts>(Vals))...));
+inline formatv_object<Ts...> formatv(const char *Fmt, Ts &&... Vals) {
+  return formatv_object<Ts...>(Fmt, std::forward<Ts>(Vals)...);
 }
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_FORMATVARIADIC_H

llvm/include/llvm/Support/FormatVariadicDetails.h

 class format_adapter {
 protected:
   virtual ~format_adapter() {}
 
 public:
   virtual void format(raw_ostream &S, StringRef Options) = 0;
 };
 
-template <typename T> class provider_format_adapter : public format_adapter {
+template <typename T> class value_holder : public format_adapter {
   T Item;
-
 public:
-  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);
+  explicit value_holder(T &&Item) : Item(Item) {}
+  virtual void format(raw_ostream &S, StringRef Options) {
+    format_one_item(std::forward<T>(Item), S, Options);
   }
 };
 
 template <typename T> class missing_format_adapter;
 
 // Test if format_provider<T> is defined on T and contains a member function
 // with the signature:
 //   static void format(const T&, raw_stream &, StringRef);

 // Simple template that decides whether a type T should use the format_provider
 // based format() invocation.  The member function takes priority, so this test
 // will only be true if there is not ALSO a format member.
 template <typename T>
 struct uses_format_provider
     : public std::integral_constant<
           bool, !uses_format_member<T>::value && has_FormatProvider<T>::value> {
 };
-
-// Simple template that decides whether a type T has neither a member-function
-// nor format_provider based implementation that it can use.  Mostly used so
-// that the compiler spits out a nice diagnostic when a type with no format
-// implementation can be located.
-template <typename T>
-struct uses_missing_provider
-    : public std::integral_constant<bool,
-                                    !uses_format_member<T>::value &&
-                                        !uses_format_provider<T>::value> {};
-
-template <typename T>
-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_adapter<T>>::type
-build_format_adapter(T &&Item) {
-  return provider_format_adapter<T>(std::forward<T>(Item));
+typename std::enable_if<detail::uses_format_member<T>::value, void>::type
+format_one_item(T &&Item, raw_ostream &S, StringRef Options) {
+  Item.format(S, Options);
 }
 
 template <typename T>
-typename std::enable_if<uses_missing_provider<T>::value,
-                        missing_format_adapter<T>>::type
-build_format_adapter(T &&Item) {
-  return missing_format_adapter<T>();
-}
+typename std::enable_if<detail::uses_format_provider<T>::value, void>::type
+format_one_item(T &&Item, raw_ostream &S, StringRef Options) {
+  format_provider<typename std::decay<T>::type>::format(std::forward<T>(Item),
+                                                        S, Options);
 }
 }
 
 #endif

llvm/unittests/Support/FormatVariadicTest.cpp

 // Compile-time tests templates in the detail namespace.
 namespace {
 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, "");
-
-struct NoFormat {};
-static_assert(uses_missing_provider<NoFormat>::value, "");
 }
 
 TEST(FormatVariadicTest, EmptyFormatString) {
   auto Replacements = formatv_object_base::parseFormatString("");
   EXPECT_EQ(0U, Replacements.size());
 }
 
 TEST(FormatVariadicTest, NoReplacements) {