Revert "make `folly::Formatter` extendible"

Summary:
This reverts commit 5ea0eb0c705224b82a5c284dc5f7722e1f71199f.

This breaks the HHVM clang build test.

Test Plan: clang will pass now

Reviewed By: smith@fb.com

Subscribers: dmitrys

FB internal diff: D1426399

Tasks: 4667712

folly/Format-inl.h

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

folly/Format.h

@@ -51,11 +51,6 @@ 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.
  *
@@ -64,12 +59,8 @@ class FormatterTag {};
  * this directly, you have to use format(...) below.
  */
 
-/* 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 {
+template <bool containerMode, class... Args>
+class Formatter {
  public:
   /*
    * Change whether or not Formatter should crash or throw exceptions if the
@@ -118,13 +109,21 @@ class BaseFormatter {
     return s;
   }
 
-  /**
-   * metadata to identify generated children of BaseFormatter
-   */
-  typedef detail::FormatterTag IsFormatter;
-  typedef BaseFormatter BaseType;
-
  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;
+
   typedef std::tuple<FormatValue<
       typename std::decay<Args>::type>...> ValueTuple;
   static constexpr size_t valueCount = std::tuple_size<ValueTuple>::value;
@@ -143,7 +142,7 @@ class BaseFormatter {
   typename std::enable_if<(K < valueCount)>::type
   doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
     if (i == K) {
-      static_cast<const Derived*>(this)->template doFormatArg<K>(arg, cb);
+      std::get<K>(values_).format(arg, cb);
     } else {
       doFormatFrom<K+1>(i, arg, cb);
     }
@@ -155,44 +154,8 @@ class BaseFormatter {
   }
 
   StringPiece str_;
-  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...>;
+  bool crashOnError_{true};
 
   template <class... A>
   friend Formatter<false, A...> format(StringPiece fmt, A&&... arg);
@@ -218,9 +181,8 @@ std::ostream& operator<<(std::ostream& out,
 /**
  * Formatter objects can be written to stdio FILEs.
  */
-template <class Derived, bool containerMode, class... Args>
-void writeTo(FILE* fp,
-             const BaseFormatter<Derived, containerMode, Args...>& formatter);
+template<bool containerMode, class... Args>
+void writeTo(FILE* fp, const Formatter<containerMode, Args...>& formatter);
 
 /**
  * Create a formatter object.
@@ -422,14 +384,10 @@ void formatNumber(StringPiece val, int prefixLen, FormatArg& arg,
  * formatString(fmt.str(), arg, cb); but avoids creating a temporary
  * string if possible.
  */
-template <class FormatCallback,
-          class Derived,
-          bool containerMode,
-          class... Args>
-void formatFormatter(
-    const BaseFormatter<Derived, containerMode, Args...>& formatter,
-    FormatArg& arg,
-    FormatCallback& cb);
+template <class FormatCallback, bool containerMode, class... Args>
+void formatFormatter(const Formatter<containerMode, Args...>& formatter,
+                     FormatArg& arg,
+                     FormatCallback& cb);
 
 }  // namespace format_value
 
@@ -455,19 +413,6 @@ void formatFormatter(
  * 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>

folly/test/FormatTest.cpp

@@ -25,8 +25,6 @@
 #include <folly/dynamic.h>
 #include <folly/json.h>
 
-#include <string>
-
 using namespace folly;
 
 template <class Uint>
@@ -381,58 +379,6 @@ 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);