From bd1cd83b305dceb9b5c5b6da1732f22ec9d2deef Mon Sep 17 00:00:00 2001 From: Tyler MacDonald Date: Mon, 21 Jul 2014 14:08:47 -0700 Subject: [PATCH] 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 --- folly/Format-inl.h | 61 +++++++++++++--------- folly/Format.h | 103 +++++++++++++++++++++++++++++--------- folly/test/FormatTest.cpp | 54 ++++++++++++++++++++ 3 files changed, 169 insertions(+), 49 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 625233f5..77248b79 100644 --- 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 -Formatter::Formatter(StringPiece str, Args&&... args) - : str_(str), - values_(FormatValue::type>( - std::forward(args))...) { +template +BaseFormatter::BaseFormatter(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 Formatter::handleFormatStrError() const { +template +void BaseFormatter::handleFormatStrError() + const { if (crashOnError_) { LOG(FATAL) << "folly::format: bad format string \"" << str_ << "\": " << folly::exceptionStr(std::current_exception()); @@ -160,9 +161,10 @@ void Formatter::handleFormatStrError() const { throw; } -template +template template -void Formatter::operator()(Output& out) const { +void BaseFormatter::operator()(Output& out) + const { // Catch BadFormatArg and range_error exceptions, and call // handleFormatStrError(). // @@ -193,9 +195,10 @@ void Formatter::operator()(Output& out) const { } } -template +template template -void Formatter::appendOutput(Output& out) const { +void BaseFormatter::appendOutput(Output& out) + const { auto p = str_.begin(); auto end = str_.end(); @@ -287,8 +290,9 @@ void Formatter::appendOutput(Output& out) const { } } -template -void writeTo(FILE* fp, const Formatter& formatter) { +template +void writeTo(FILE* fp, + const BaseFormatter& 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 -void formatFormatter(const Formatter& formatter, - FormatArg& arg, - FormatCallback& cb) { +template +void formatFormatter( + const BaseFormatter& formatter, + FormatArg& arg, + FormatCallback& cb) { if (arg.width == FormatArg::kDefaultWidth && arg.precision == FormatArg::kDefaultPrecision) { // nothing to do @@ -1204,9 +1212,13 @@ class FormatValue> { }; // Partial specialization of FormatValue for nested Formatters -template -class FormatValue, void> { - typedef Formatter FormatterValue; +template class F> +class FormatValue, + typename std::enable_if>::value>::type> { + typedef typename F::BaseType FormatterValue; + public: explicit FormatValue(const FormatterValue& f) : f_(f) { } @@ -1222,10 +1234,9 @@ class FormatValue, void> { * Formatter objects can be appended to strings, and therefore they're * compatible with folly::toAppend and folly::to. */ -template -typename std::enable_if< - IsSomeString::value>::type -toAppend(const Formatter& value, Tgt * result) { +template +typename std::enable_if::value>::type toAppend( + const BaseFormatter& value, Tgt* result) { value.appendTo(*result); } diff --git a/folly/Format.h b/folly/Format.h index 343f7b53..bbfb54c7 100644 --- a/folly/Format.h +++ b/folly/Format.h @@ -51,6 +51,11 @@ 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. * @@ -59,8 +64,12 @@ template class FormatValue; * this directly, you have to use format(...) below. */ -template -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 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::type>...> ValueTuple; static constexpr size_t valueCount = std::tuple_size::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(values_).format(arg, cb); + static_cast(this)->template doFormatArg(arg, cb); } else { doFormatFrom(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 +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...>; + template friend Formatter format(StringPiece fmt, A&&... arg); template @@ -181,8 +218,9 @@ std::ostream& operator<<(std::ostream& out, /** * Formatter objects can be written to stdio FILEs. */ -template -void writeTo(FILE* fp, const Formatter& formatter); +template +void writeTo(FILE* fp, + const BaseFormatter& 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 -void formatFormatter(const Formatter& formatter, - FormatArg& arg, - FormatCallback& cb); +template +void formatFormatter( + const BaseFormatter& formatter, + FormatArg& arg, + FormatCallback& cb); } // namespace format_value @@ -413,6 +455,19 @@ void formatFormatter(const Formatter& formatter, * 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 5e015e62..8db3f09d 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -25,6 +25,8 @@ #include #include +#include + using namespace folly; template @@ -379,6 +381,58 @@ 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