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

@@ -157,9 +157,7 @@ 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))...) {}
+    : str_(str), values_(std::forward<Args>(args)...) {}
 
 template <class Derived, bool containerMode, class... Args>
 template <class Output>
@@ -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<size_t>
-  // 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<size_t>(arg.precision)) {
-    val.reset(val.data(), size_t(arg.precision));
+    val.reset(val.data(), static_cast<size_t>(arg.precision));
   }
 
   constexpr int padBufSize = 128;
@@ -684,7 +678,7 @@ class FormatValue<float> {
   float val_;
 };
 
-// Sring-y types (implicitly convertible to StringPiece, except char*)
+// String-y types (implicitly convertible to StringPiece, except char*)
 template <class T>
 class FormatValue<
     T,

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<FormatValue<typename std::decay<Args>::type>...>
-      ValueTuple;
+  typedef std::tuple<Args...> ValueTuple;
   static constexpr size_t valueCount = std::tuple_size<ValueTuple>::value;
 
   Derived const& asDerived() const {
@@ -166,7 +166,7 @@ class BaseFormatter {
       K<valueCount, int>::type getSizeArgFrom(size_t i, const FormatArg& arg)
           const {
     if (i == K) {
-      return getValue(std::get<K>(values_), arg);
+      return getValue(getFormatValue<K>(), arg);
     }
     return getSizeArgFrom<K + 1>(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 <size_t K>
+  using ArgType = typename std::tuple_element<K, ValueTuple>::type;
+
+  template <size_t K>
+  FormatValue<typename std::decay<ArgType<K>>::type> getFormatValue() const {
+    return FormatValue<typename std::decay<ArgType<K>>::type>(
+        std::get<K>(values_));
+  }
+
   ValueTuple values_;
 };
 
@@ -213,7 +222,7 @@ class Formatter : public BaseFormatter<
 
   template <size_t K, class Callback>
   void doFormatArg(FormatArg& arg, Callback& cb) const {
-    std::get<K>(this->values_).format(arg, cb);
+    this->template getFormatValue<K>().format(arg, cb);
   }
 
   friend class BaseFormatter<

folly/test/FormatTest.cpp

@@ -15,7 +15,7 @@
  */
 
 #include <folly/Format.h>
-
+#include <folly/Utility.h>
 #include <folly/portability/GTest.h>
 
 #include <string>
@@ -352,7 +352,7 @@ template <> class FormatValue<KeyValue> {
   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<K>(this->values_).format(arg, appender);
+    this->template getFormatValue<K>().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<NoncopyableInt> {
+ public:
+  explicit FormatValue(const NoncopyableInt& v) : v_(v) {}
+
+  template <class FormatCallback>
+  void format(FormatArg& arg, FormatCallback& cb) const {
+    FormatValue<int>(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");
+  }
+}