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
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>
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;
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,
/**
* 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.
}
/**
- * 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 {
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);
}
// 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_;
};
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<
*/
#include <folly/Format.h>
-
+#include <folly/Utility.h>
#include <folly/portability/GTest.h>
#include <string>
const KeyValue& kv_;
};
-} // namespace
+} // namespace folly
TEST(Format, Custom) {
KeyValue kv { "hello", 42 };
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));
}
"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");
+ }
+}
projects / folly.git / commitdiff