From 2c378ec80fa59964ec7c143ec2a9b53fc6a1fd15 Mon Sep 17 00:00:00 2001 From: Paul Tarjan Date: Wed, 9 Jul 2014 07:07:38 -0700 Subject: [PATCH] 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 | 62 +++++++++-------------- folly/Format.h | 103 +++++++++----------------------------- folly/test/FormatTest.cpp | 54 -------------------- 3 files changed, 49 insertions(+), 170 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 7750be05..625233f5 100644 --- a/folly/Format-inl.h +++ b/folly/Format-inl.h @@ -141,19 +141,18 @@ size_t uintToBinary(char* buffer, size_t bufLen, Uint v) { } // namespace detail -template -BaseFormatter::BaseFormatter(StringPiece str, - Args&&... args) - : str_(str), - values_(FormatValue::type>( - std::forward(args))...) { + +template +Formatter::Formatter(StringPiece str, Args&&... args) + : str_(str), + values_(FormatValue::type>( + std::forward(args))...) { static_assert(!containerMode || sizeof...(Args) == 1, "Exactly one argument required in container mode"); } -template -void BaseFormatter::handleFormatStrError() - const { +template +void Formatter::handleFormatStrError() const { if (crashOnError_) { LOG(FATAL) << "folly::format: bad format string \"" << str_ << "\": " << folly::exceptionStr(std::current_exception()); @@ -161,10 +160,9 @@ void BaseFormatter::handleFormatStrError() throw; } -template +template template -void BaseFormatter::operator()(Output& out) - const { +void Formatter::operator()(Output& out) const { // Catch BadFormatArg and range_error exceptions, and call // handleFormatStrError(). // @@ -195,10 +193,9 @@ void BaseFormatter::operator()(Output& out) } } -template +template template -void BaseFormatter::appendOutput(Output& out) - const { +void Formatter::appendOutput(Output& out) const { auto p = str_.begin(); auto end = str_.end(); @@ -290,9 +287,8 @@ void BaseFormatter::appendOutput(Output& out) } } -template -void writeTo(FILE* fp, - const BaseFormatter& formatter) { +template +void writeTo(FILE* fp, const Formatter& 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 -void formatFormatter( - const BaseFormatter& formatter, - FormatArg& arg, - FormatCallback& cb) { +template +void formatFormatter(const Formatter& formatter, + FormatArg& arg, + FormatCallback& cb) { if (arg.width == FormatArg::kDefaultWidth && arg.precision == FormatArg::kDefaultPrecision) { // nothing to do @@ -1212,14 +1204,9 @@ class FormatValue> { }; // Partial specialization of FormatValue for nested Formatters -template class F> -class FormatValue, - typename std::enable_if>::value>::type> { - typedef typename F::BaseType FormatterValue; - +template +class FormatValue, void> { + typedef Formatter FormatterValue; public: explicit FormatValue(const FormatterValue& f) : f_(f) { } @@ -1235,9 +1222,10 @@ class FormatValue, * Formatter objects can be appended to strings, and therefore they're * compatible with folly::toAppend and folly::to. */ -template -typename std::enable_if::value>::type toAppend( - const BaseFormatter& value, Tgt* result) { +template +typename std::enable_if< + IsSomeString::value>::type +toAppend(const Formatter& value, Tgt * result) { value.appendTo(*result); } diff --git a/folly/Format.h b/folly/Format.h index bbfb54c7..343f7b53 100644 --- a/folly/Format.h +++ b/folly/Format.h @@ -51,11 +51,6 @@ template Formatter vformat(StringPiece fmt, C&& container); template 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 BaseFormatter { +template +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::type>...> ValueTuple; static constexpr size_t valueCount = std::tuple_size::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(this)->template doFormatArg(arg, cb); + std::get(values_).format(arg, cb); } else { doFormatFrom(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 -class Formatter : public BaseFormatter, - containerMode, - Args...> { - private: - explicit Formatter(StringPiece& str, Args&&... args) - : BaseFormatter, - containerMode, - Args...>(str, std::forward(args)...) {} - - template - void doFormatArg(FormatArg& arg, Callback& cb) const { - std::get(this->values_).format(arg, cb); - } - - friend class BaseFormatter, - containerMode, - Args...>; + bool crashOnError_{true}; template friend Formatter format(StringPiece fmt, A&&... arg); @@ -218,9 +181,8 @@ std::ostream& operator<<(std::ostream& out, /** * Formatter objects can be written to stdio FILEs. */ -template -void writeTo(FILE* fp, - const BaseFormatter& formatter); +template +void writeTo(FILE* fp, const Formatter& 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 -void formatFormatter( - const BaseFormatter& formatter, - FormatArg& arg, - FormatCallback& cb); +template +void formatFormatter(const Formatter& formatter, + FormatArg& arg, + FormatCallback& cb); } // namespace format_value @@ -455,19 +413,6 @@ void formatFormatter( * empty string) */ -namespace detail { - -template -struct IsFormatter : public std::false_type {}; - -template -struct IsFormatter< - T, - typename std::enable_if< - std::is_same::value>:: - type> : public std::true_type {}; -} // folly::detail - } // namespace folly #include diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp index 8db3f09d..5e015e62 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -25,8 +25,6 @@ #include #include -#include - using namespace folly; template @@ -381,58 +379,6 @@ TEST(Format, BogusFormatString) { EXPECT_THROW(sformatChecked("{0[test}"), std::exception); } -template -class TestExtendingFormatter; - -template -class TestExtendingFormatter - : public BaseFormatter, - containerMode, - Args...> { - private: - explicit TestExtendingFormatter(StringPiece& str, Args&&... args) - : BaseFormatter, - containerMode, - Args...>(str, std::forward(args)...) {} - - template - void doFormatArg(FormatArg& arg, Callback& cb) const { - std::string result; - auto appender = [&result](StringPiece s) { - result.append(s.data(), s.size()); - }; - std::get(this->values_).format(arg, appender); - result = sformat("{{{}}}", result); - cb(StringPiece(result)); - } - - friend class BaseFormatter, - containerMode, - Args...>; - - template - friend std::string texsformat(StringPiece fmt, A&&... arg); -}; - -template -std::string texsformat(StringPiece fmt, Args&&... args) { - return TestExtendingFormatter( - fmt, std::forward(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); -- 2.34.1