fix D1422343 ("make `folly::Formatter` extendible") for clang

Summary:
This is D1422343, but with a one-line change to make it clang-compatible. I authored this diff by first copying the original, then updating, so it's easy to see what I had to change.

> on advice of @tudorb, move most of `folly::Formatter` into `folly::BaseFormatter` so that we can use compile-time polymorphism to provide different types of Formatters.

Test Plan:
```
fbmake clean
fbconfig -r thrift folly cold_storage && fbmake dbg && fbmake runtests
fbmake clean
fbconfig -r hphp --clang && fbmake dbgo && fbmake runtests
```

Reviewed By: tudorb@fb.com, pt@fb.com

Subscribers: mathieubaudet, tudorb

FB internal diff: D1440310

Tasks: 4624268, 4667712

diff --git a/folly/Format-inl.h b/folly/Format-inl.h
index 625233f5f49c6a97e1045761a8bcae88f2a626b1..77248b79dad43f62d208cb17d7fe19524bc766c1 100644 (file)
--- a/folly/Format-inl.h
+++ b/folly/Format-inl.h
@@ -141,18 +141,19 @@ size_t uintToBinary(char* buffer, size_t bufLen, Uint v) {
 
 }  // namespace detail
 
-
-template <bool containerMode, class... Args>
-Formatter<containerMode, Args...>::Formatter(StringPiece str, Args&&... args)
-  : str_(str),
-    values_(FormatValue<typename std::decay<Args>::type>(
-        std::forward<Args>(args))...) {
+template <class Derived, bool containerMode, class... Args>
+BaseFormatter<Derived, containerMode, Args...>::BaseFormatter(StringPiece str,
+                                                              Args&&... args)
+    : str_(str),
+      values_(FormatValue<typename std::decay<Args>::type>(
+          std::forward<Args>(args))...) {
   static_assert(!containerMode || sizeof...(Args) == 1,
                 "Exactly one argument required in container mode");
 }
 
-template <bool containerMode, class... Args>
-void Formatter<containerMode, Args...>::handleFormatStrError() const {
+template <class Derived, bool containerMode, class... Args>
+void BaseFormatter<Derived, containerMode, Args...>::handleFormatStrError()
+    const {
   if (crashOnError_) {
     LOG(FATAL) << "folly::format: bad format string \"" << str_ << "\": " <<
       folly::exceptionStr(std::current_exception());
@@ -160,9 +161,10 @@ void Formatter<containerMode, Args...>::handleFormatStrError() const {
   throw;
 }
 
-template <bool containerMode, class... Args>
+template <class Derived, bool containerMode, class... Args>
 template <class Output>
-void Formatter<containerMode, Args...>::operator()(Output& out) const {
+void BaseFormatter<Derived, containerMode, Args...>::operator()(Output& out)
+    const {
   // Catch BadFormatArg and range_error exceptions, and call
   // handleFormatStrError().
   //
@@ -193,9 +195,10 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
   }
 }
 
-template <bool containerMode, class... Args>
+template <class Derived, bool containerMode, class... Args>
 template <class Output>
-void Formatter<containerMode, Args...>::appendOutput(Output& out) const {
+void BaseFormatter<Derived, containerMode, Args...>::appendOutput(Output& out)
+    const {
   auto p = str_.begin();
   auto end = str_.end();
 
@@ -287,8 +290,9 @@ void Formatter<containerMode, Args...>::appendOutput(Output& out) const {
   }
 }
 
-template <bool containerMode, class... Args>
-void writeTo(FILE* fp, const Formatter<containerMode, Args...>& formatter) {
+template <class Derived, bool containerMode, class... Args>
+void writeTo(FILE* fp,
+             const BaseFormatter<Derived, containerMode, Args...>& formatter) {
   auto writer = [fp] (StringPiece sp) {
     ssize_t n = fwrite(sp.data(), 1, sp.size(), fp);
     if (n < sp.size()) {
@@ -367,10 +371,14 @@ void formatNumber(StringPiece val, int prefixLen, FormatArg& arg,
   format_value::formatString(val, arg, cb);
 }
 
-template <class FormatCallback, bool containerMode, class... Args>
-void formatFormatter(const Formatter<containerMode, Args...>& formatter,
-                     FormatArg& arg,
-                     FormatCallback& cb) {
+template <class FormatCallback,
+          class Derived,
+          bool containerMode,
+          class... Args>
+void formatFormatter(
+    const BaseFormatter<Derived, containerMode, Args...>& formatter,
+    FormatArg& arg,
+    FormatCallback& cb) {
   if (arg.width == FormatArg::kDefaultWidth &&
       arg.precision == FormatArg::kDefaultPrecision) {
     // nothing to do
@@ -1204,9 +1212,13 @@ class FormatValue<std::tuple<Args...>> {
 };
 
 // Partial specialization of FormatValue for nested Formatters
-template <bool containerMode, class... Args>
-class FormatValue<Formatter<containerMode, Args...>, void> {
-  typedef Formatter<containerMode, Args...> FormatterValue;
+template <bool containerMode, class... Args,
+          template <bool, class...> class F>
+class FormatValue<F<containerMode, Args...>,
+                  typename std::enable_if<detail::IsFormatter<
+                      F<containerMode, Args...>>::value>::type> {
+  typedef typename F<containerMode, Args...>::BaseType FormatterValue;
+
  public:
   explicit FormatValue(const FormatterValue& f) : f_(f) { }
 
@@ -1222,10 +1234,9 @@ class FormatValue<Formatter<containerMode, Args...>, void> {
  * Formatter objects can be appended to strings, and therefore they're
  * compatible with folly::toAppend and folly::to.
  */
-template <class Tgt, bool containerMode, class... Args>
-typename std::enable_if<
-   IsSomeString<Tgt>::value>::type
-toAppend(const Formatter<containerMode, Args...>& value, Tgt * result) {
+template <class Tgt, class Derived, bool containerMode, class... Args>
+typename std::enable_if<IsSomeString<Tgt>::value>::type toAppend(
+    const BaseFormatter<Derived, containerMode, Args...>& value, Tgt* result) {
   value.appendTo(*result);
 }
 

diff --git a/folly/Format.h b/folly/Format.h
index 343f7b534f674e760c1bbf5674ab6f372eaeaa8d..bbfb54c743d1a4aedbcbe97ae41d08ed7335db90 100644 (file)
--- a/folly/Format.h
+++ b/folly/Format.h
@@ -51,6 +51,11 @@ template <class C>
 Formatter<true, C> vformat(StringPiece fmt, C&& container);
 template <class T, class Enable=void> class FormatValue;
 
+// meta-attribute to identify formatters in this sea of template weirdness
+namespace detail {
+class FormatterTag {};
+};
+
 /**
  * Formatter class.
  *
@@ -59,8 +64,12 @@ template <class T, class Enable=void> class FormatValue;
  * this directly, you have to use format(...) below.
  */
 
-template <bool containerMode, class... Args>
-class Formatter {
+/* BaseFormatter class. Currently, the only behavior that can be
+ * overridden is the actual formatting of positional parameters in
+ * `doFormatArg`. The Formatter class provides the default implementation.
+ */
+template <class Derived, bool containerMode, class... Args>
+class BaseFormatter {
  public:
   /*
    * Change whether or not Formatter should crash or throw exceptions if the
@@ -109,21 +118,13 @@ class Formatter {
     return s;
   }
 
- private:
-  explicit Formatter(StringPiece str, Args&&... args);
-
-  // Not copyable
-  Formatter(const Formatter&) = delete;
-  Formatter& operator=(const Formatter&) = delete;
-
-  // Movable, but the move constructor and assignment operator are private,
-  // for the exclusive use of format() (below).  This way, you can't create
-  // a Formatter object, but can handle references to it (for streaming,
-  // conversion to string, etc) -- which is good, as Formatter objects are
-  // dangerous (they hold references, possibly to temporaries)
-  Formatter(Formatter&&) = default;
-  Formatter& operator=(Formatter&&) = default;
+  /**
+   * metadata to identify generated children of BaseFormatter
+   */
+  typedef detail::FormatterTag IsFormatter;
+  typedef BaseFormatter BaseType;
 
+ private:
   typedef std::tuple<FormatValue<
       typename std::decay<Args>::type>...> ValueTuple;
   static constexpr size_t valueCount = std::tuple_size<ValueTuple>::value;
@@ -142,7 +143,7 @@ class Formatter {
   typename std::enable_if<(K < valueCount)>::type
   doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
     if (i == K) {
-      std::get<K>(values_).format(arg, cb);
+      static_cast<const Derived*>(this)->template doFormatArg<K>(arg, cb);
     } else {
       doFormatFrom<K+1>(i, arg, cb);
     }
@@ -154,9 +155,45 @@ class Formatter {
   }
 
   StringPiece str_;
-  ValueTuple values_;
   bool crashOnError_{true};
 
+ protected:
+  explicit BaseFormatter(StringPiece str, Args&&... args);
+
+  // Not copyable
+  BaseFormatter(const BaseFormatter&) = delete;
+  BaseFormatter& operator=(const BaseFormatter&) = delete;
+
+  // Movable, but the move constructor and assignment operator are private,
+  // for the exclusive use of format() (below).  This way, you can't create
+  // a Formatter object, but can handle references to it (for streaming,
+  // conversion to string, etc) -- which is good, as Formatter objects are
+  // dangerous (they hold references, possibly to temporaries)
+  BaseFormatter(BaseFormatter&&) = default;
+  BaseFormatter& operator=(BaseFormatter&&) = default;
+
+  ValueTuple values_;
+};
+
+template <bool containerMode, class... Args>
+class Formatter : public BaseFormatter<Formatter<containerMode, Args...>,
+                                       containerMode,
+                                       Args...> {
+ private:
+  explicit Formatter(StringPiece& str, Args&&... args)
+      : BaseFormatter<Formatter<containerMode, Args...>,
+                      containerMode,
+                      Args...>(str, std::forward<Args>(args)...) {}
+
+  template <size_t K, class Callback>
+  void doFormatArg(FormatArg& arg, Callback& cb) const {
+    std::get<K>(this->values_).format(arg, cb);
+  }
+
+  friend class BaseFormatter<Formatter<containerMode, Args...>,
+                             containerMode,
+                             Args...>;
+
   template <class... A>
   friend Formatter<false, A...> format(StringPiece fmt, A&&... arg);
   template <class... A>
@@ -181,8 +218,9 @@ std::ostream& operator<<(std::ostream& out,
 /**
  * Formatter objects can be written to stdio FILEs.
  */
-template<bool containerMode, class... Args>
-void writeTo(FILE* fp, const Formatter<containerMode, Args...>& formatter);
+template <class Derived, bool containerMode, class... Args>
+void writeTo(FILE* fp,
+             const BaseFormatter<Derived, containerMode, Args...>& formatter);
 
 /**
  * Create a formatter object.
@@ -384,10 +422,14 @@ void formatNumber(StringPiece val, int prefixLen, FormatArg& arg,
  * formatString(fmt.str(), arg, cb); but avoids creating a temporary
  * string if possible.
  */
-template <class FormatCallback, bool containerMode, class... Args>
-void formatFormatter(const Formatter<containerMode, Args...>& formatter,
-                     FormatArg& arg,
-                     FormatCallback& cb);
+template <class FormatCallback,
+          class Derived,
+          bool containerMode,
+          class... Args>
+void formatFormatter(
+    const BaseFormatter<Derived, containerMode, Args...>& formatter,
+    FormatArg& arg,
+    FormatCallback& cb);
 
 }  // namespace format_value
 
@@ -413,6 +455,19 @@ void formatFormatter(const Formatter<containerMode, Args...>& formatter,
  * empty string)
  */
 
+namespace detail {
+
+template <class T, class Enable = void>
+struct IsFormatter : public std::false_type {};
+
+template <class T>
+struct IsFormatter<
+    T,
+    typename std::enable_if<
+        std::is_same<typename T::IsFormatter, detail::FormatterTag>::value>::
+        type> : public std::true_type {};
+} // folly::detail
+
 }  // namespace folly
 
 #include <folly/Format-inl.h>

diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp
index 5e015e62aca63322ed9f38124e0347812e7de4bc..8db3f09d2d6b9d8cccc2639b6938b5b9d191d28b 100644 (file)
--- a/folly/test/FormatTest.cpp
+++ b/folly/test/FormatTest.cpp
@@ -25,6 +25,8 @@
 #include <folly/dynamic.h>
 #include <folly/json.h>
 
+#include <string>
+
 using namespace folly;
 
 template <class Uint>
@@ -379,6 +381,58 @@ TEST(Format, BogusFormatString) {
   EXPECT_THROW(sformatChecked("{0[test}"), std::exception);
 }
 
+template <bool containerMode, class... Args>
+class TestExtendingFormatter;
+
+template <bool containerMode, class... Args>
+class TestExtendingFormatter
+    : public BaseFormatter<TestExtendingFormatter<containerMode, Args...>,
+                           containerMode,
+                           Args...> {
+ private:
+  explicit TestExtendingFormatter(StringPiece& str, Args&&... args)
+      : BaseFormatter<TestExtendingFormatter<containerMode, Args...>,
+                      containerMode,
+                      Args...>(str, std::forward<Args>(args)...) {}
+
+  template <size_t K, class Callback>
+  void doFormatArg(FormatArg& arg, Callback& cb) const {
+    std::string result;
+    auto appender = [&result](StringPiece s) {
+      result.append(s.data(), s.size());
+    };
+    std::get<K>(this->values_).format(arg, appender);
+    result = sformat("{{{}}}", result);
+    cb(StringPiece(result));
+  }
+
+  friend class BaseFormatter<TestExtendingFormatter<containerMode, Args...>,
+                             containerMode,
+                             Args...>;
+
+  template <class... A>
+  friend std::string texsformat(StringPiece fmt, A&&... arg);
+};
+
+template <class... Args>
+std::string texsformat(StringPiece fmt, Args&&... args) {
+  return TestExtendingFormatter<false, Args...>(
+      fmt, std::forward<Args>(args)...).str();
+}
+
+TEST(Format, Extending) {
+  EXPECT_EQ(texsformat("I {} brackets", "love"), "I {love} brackets");
+  EXPECT_EQ(texsformat("I {} nesting", sformat("really {}", "love")),
+            "I {really love} nesting");
+  EXPECT_EQ(
+      sformat("I also {} nesting", texsformat("have an {} for", "affinity")),
+      "I also have an {affinity} for nesting");
+  EXPECT_EQ(texsformat("Extending {} in {}",
+                       texsformat("a {}", "formatter"),
+                       "another formatter"),
+            "Extending {a {formatter}} in {another formatter}");
+}
+
 int main(int argc, char *argv[]) {
   testing::InitGoogleTest(&argc, argv);
   gflags::ParseCommandLineFlags(&argc, &argv, true);