From 2734e379f411f7d9757f9b8d13c020249a6c2dd6 Mon Sep 17 00:00:00 2001 From: Giuseppe Ottaviano Date: Mon, 26 Jun 2017 14:42:31 -0700 Subject: [PATCH] Improve Format's handling of temporaries Summary: `FormatValue` holds non-integral objects by reference, which can cause subtle problems if a formatter is constructed with temporary arguments and used beyond the expression that constructs it. With this diff the arguments are perfectly forwarded into a tuple, so the formatter will take ownership of the temporaries while holding references to lvalues as before. The only downside is that now `FormatValue` objects are constructed every time the formatter is used, rather than only at formatter construction. This should not be noticeable as those objects' constructors should just take a reference to the argument. Note that the format string is still held by reference, but this is fine because it should almost always be a string literal. Reviewed By: ericniebler Differential Revision: D5317382 fbshipit-source-id: ef8355194b634d3751ef1ccca32dd1db29e27c48 --- folly/Format-inl.h | 12 ++------ folly/Format.h | 27 ++++++++++++------ folly/test/FormatTest.cpp | 58 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 21 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 828de961..7cea87a4 100644 --- a/folly/Format-inl.h +++ b/folly/Format-inl.h @@ -157,9 +157,7 @@ template BaseFormatter::BaseFormatter( StringPiece str, Args&&... args) - : str_(str), - values_(FormatValue::type>( - std::forward(args))...) {} + : str_(str), values_(std::forward(args)...) {} template template @@ -298,13 +296,9 @@ void formatString(StringPiece val, FormatArg& arg, FormatCallback& cb) { throw BadFormatArg("folly::format: invalid precision"); } - // XXX: clang should be smart enough to not need the two static_cast - // uses below given the above checks. If clang ever becomes that smart, we - // should remove the otherwise unnecessary warts. - if (arg.precision != FormatArg::kDefaultPrecision && val.size() > static_cast(arg.precision)) { - val.reset(val.data(), size_t(arg.precision)); + val.reset(val.data(), static_cast(arg.precision)); } constexpr int padBufSize = 128; @@ -684,7 +678,7 @@ class FormatValue { float val_; }; -// Sring-y types (implicitly convertible to StringPiece, except char*) +// String-y types (implicitly convertible to StringPiece, except char*) template class FormatValue< T, diff --git a/folly/Format.h b/folly/Format.h index 7c9929a9..d6fd2ff0 100644 --- a/folly/Format.h +++ b/folly/Format.h @@ -51,9 +51,10 @@ class FormatterTag {}; /** * Formatter class. * - * Note that this class is tricky, as it keeps *references* to its arguments - * (and doesn't copy the passed-in format string). Thankfully, you can't use - * this directly, you have to use format(...) below. + * Note that this class is tricky, as it keeps *references* to its lvalue + * arguments (while it takes ownership of the temporaries), and it doesn't + * copy the passed-in format string. Thankfully, you can't use this + * directly, you have to use format(...) below. */ /* BaseFormatter class. @@ -103,14 +104,13 @@ class BaseFormatter { } /** - * metadata to identify generated children of BaseFormatter + * Metadata to identify generated children of BaseFormatter */ typedef detail::FormatterTag IsFormatter; typedef BaseFormatter BaseType; private: - typedef std::tuple::type>...> - ValueTuple; + typedef std::tuple ValueTuple; static constexpr size_t valueCount = std::tuple_size::value; Derived const& asDerived() const { @@ -166,7 +166,7 @@ class BaseFormatter { K::type getSizeArgFrom(size_t i, const FormatArg& arg) const { if (i == K) { - return getValue(std::get(values_), arg); + return getValue(getFormatValue(), arg); } return getSizeArgFrom(i, arg); } @@ -188,10 +188,19 @@ class BaseFormatter { // 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) + // dangerous (they may hold references). BaseFormatter(BaseFormatter&&) = default; BaseFormatter& operator=(BaseFormatter&&) = default; + template + using ArgType = typename std::tuple_element::type; + + template + FormatValue>::type> getFormatValue() const { + return FormatValue>::type>( + std::get(values_)); + } + ValueTuple values_; }; @@ -213,7 +222,7 @@ class Formatter : public BaseFormatter< template void doFormatArg(FormatArg& arg, Callback& cb) const { - std::get(this->values_).format(arg, cb); + this->template getFormatValue().format(arg, cb); } friend class BaseFormatter< diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp index 5a342226..8315c16d 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -15,7 +15,7 @@ */ #include - +#include #include #include @@ -352,7 +352,7 @@ template <> class FormatValue { const KeyValue& kv_; }; -} // namespace +} // namespace folly TEST(Format, Custom) { KeyValue kv { "hello", 42 }; @@ -477,7 +477,7 @@ class TestExtendingFormatter auto appender = [&result](StringPiece s) { result.append(s.data(), s.size()); }; - std::get(this->values_).format(arg, appender); + this->template getFormatValue().format(arg, appender); result = sformat("{{{}}}", result); cb(StringPiece(result)); } @@ -508,3 +508,55 @@ TEST(Format, Extending) { "another formatter"), "Extending {a {formatter}} in {another formatter}"); } + +TEST(Format, Temporary) { + constexpr StringPiece kStr = "A long string that should go on the heap"; + auto fmt = format("{}", kStr.str()); // Pass a temporary std::string. + EXPECT_EQ(fmt.str(), kStr); + // The formatter can be reused. + EXPECT_EQ(fmt.str(), kStr); +} + +namespace { + +struct NoncopyableInt : MoveOnly { + explicit NoncopyableInt(int v) : value(v) {} + int value; +}; + +} // namespace + +namespace folly { + +template <> +class FormatValue { + public: + explicit FormatValue(const NoncopyableInt& v) : v_(v) {} + + template + void format(FormatArg& arg, FormatCallback& cb) const { + FormatValue(v_.value).format(arg, cb); + } + + private: + const NoncopyableInt& v_; +}; + +} // namespace + +TEST(Format, NoncopyableArg) { + { + // Test that lvalues are held by reference. + NoncopyableInt v(1); + auto fmt = format("{}", v); + EXPECT_EQ(fmt.str(), "1"); + // The formatter can be reused. + EXPECT_EQ(fmt.str(), "1"); + } + + { + // Test that rvalues are moved. + auto fmt = format("{}", NoncopyableInt(1)); + EXPECT_EQ(fmt.str(), "1"); + } +} -- 2.34.1