Commit 4e249e08 authored by Victor Zverovich's avatar Victor Zverovich Committed by Facebook GitHub Bot

Deprecate folly::format

Summary:
`folly::format` is a problematic API because it returns a `Formatter` object that may store references to arguments as its comment warns:

```
 * Formatter class.
 *
 * 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.
```

This has negative safety and performance (encourages reuse of the same formatter) implications because contrary to what the comment says you can use the object directly since it's returned from `folly::format`. For example
```
auto f = folly::format(std::string("{}"), 42);
f.str();
```
is a UB with no compile-time diagnostic (only caught by ASAN).

It's also unnecessary because the `Formatter` object is usually converted to string straight away via `str()` or written to an output stream. Reusing the formatter doesn't make much sense either because the expensive part is formatting, not capturing arguments.

This diff deprecates `folly::format` suggesting `fmt::format` as a potential replacement. `fmt::format` doesn't have the above problem because arguments are always in scope. It also has the following advantages:

* Better compile times.
* Compile-time format string checks.
* Compatibility with C++20 `std::format`.
* Better performance, particularly with format string compilation which eliminates format string processing overhead altogether.
* Smaller binary footprint.

Also remove `folly::writeTo` which is no longer used and the `format_nested_fbstrings` benchmark which is identical to `format_nested_strings` since there is no `fbstr()` any more.

Reviewed By: yfeldblum

Differential Revision: D26391489

fbshipit-source-id: f0309e78db0eb6d8c22b426d4cc333a17c53f73e
parent 2e670e0a
......@@ -277,18 +277,6 @@ void BaseFormatter<Derived, containerMode, Args...>::operator()(
}
}
template <class Derived, bool containerMode, class... Args>
void writeTo(
FILE* fp, const BaseFormatter<Derived, containerMode, Args...>& formatter) {
auto writer = [fp](StringPiece sp) {
size_t n = fwrite(sp.data(), 1, sp.size(), fp);
if (n < sp.size()) {
throwSystemError("Formatter writeTo", "fwrite failed");
}
};
formatter(writer);
}
namespace format_value {
template <class FormatCallback>
......
......@@ -224,6 +224,11 @@ class Formatter : public BaseFormatter<
template <class... A>
friend Formatter<false, A...> format(StringPiece fmt, A&&... arg);
template <class Str, class... A>
friend typename std::enable_if<IsSomeString<Str>::value>::type format(
Str* out, StringPiece fmt, A&&... args);
template <class... A>
friend std::string sformat(StringPiece fmt, A&&... arg);
template <class C>
friend Formatter<true, C> vformat(StringPiece fmt, C&& container);
};
......@@ -250,7 +255,11 @@ std::ostream& operator<<(
* writeTo(stdout, format("{} {}", 23, 42));
*/
template <class... Args>
Formatter<false, Args...> format(StringPiece fmt, Args&&... args) {
[[deprecated(
"Use fmt::format instead of folly::format for better performance, build "
"times and compatibility with std::format")]] //
Formatter<false, Args...>
format(StringPiece fmt, Args&&... args) {
return Formatter<false, Args...>(fmt, std::forward<Args>(args)...);
}
......@@ -260,7 +269,7 @@ Formatter<false, Args...> format(StringPiece fmt, Args&&... args) {
*/
template <class... Args>
inline std::string sformat(StringPiece fmt, Args&&... args) {
return format(fmt, std::forward<Args>(args)...).str();
return Formatter<false, Args...>(fmt, std::forward<Args>(args)...).str();
}
/**
......@@ -346,7 +355,7 @@ detail::DefaultValueWrapper<Container, Value> defaulted(
template <class Str, class... Args>
typename std::enable_if<IsSomeString<Str>::value>::type format(
Str* out, StringPiece fmt, Args&&... args) {
format(fmt, std::forward<Args>(args)...).appendTo(*out);
Formatter<false, Args...>(fmt, std::forward<Args>(args)...).appendTo(*out);
}
/**
......
......@@ -23,10 +23,10 @@
#include <string>
#include <boost/intrusive/parent_from_member.hpp>
#include <fmt/ostream.h>
#include <glog/logging.h>
#include <folly/Exception.h>
#include <folly/Format.h>
#include <folly/Likely.h>
#include <folly/String.h>
#include <folly/portability/Unistd.h>
......@@ -56,7 +56,8 @@ const char* iocbCmdToString(short int cmd_short) {
#undef X
void toStream(std::ostream& os, const iocb& cb) {
os << folly::format(
fmt::print(
os,
"data={}, key={}, opcode={}, reqprio={}, fd={}, f={}, ",
cb.data,
cb.key,
......@@ -68,7 +69,8 @@ void toStream(std::ostream& os, const iocb& cb) {
switch (cb.aio_lio_opcode) {
case IO_CMD_PREAD:
case IO_CMD_PWRITE:
os << folly::format(
fmt::print(
os,
"buf={}, offset={}, nbytes={}, ",
cb.u.c.buf,
cb.u.c.offset,
......
......@@ -19,7 +19,7 @@
#include <glog/logging.h>
#include <folly/Format.h>
#include <fmt/core.h>
#include <folly/Range.h>
#include <folly/experimental/io/HugePages.h>
#include <folly/portability/GFlags.h>
......@@ -32,7 +32,8 @@ using namespace folly;
namespace {
[[noreturn]] void usage(const char* name) {
std::cerr << folly::format(
fmt::print(
stderr,
"Usage: {0}\n"
" list all huge page sizes and their mount points\n"
" {0} -cp <src_file> <dest_nameprefix>\n"
......
......@@ -23,10 +23,10 @@
#include <string>
#include <boost/intrusive/parent_from_member.hpp>
#include <fmt/ostream.h>
#include <glog/logging.h>
#include <folly/Exception.h>
#include <folly/Format.h>
#include <folly/Likely.h>
#include <folly/String.h>
#include <folly/portability/Unistd.h>
......@@ -72,7 +72,8 @@ const char* ioUringOpToString(unsigned char op) {
#undef X
void toStream(std::ostream& os, const struct io_uring_sqe& sqe) {
os << folly::format(
fmt::print(
os,
"user_data={}, opcode={}, ioprio={}, f={}, ",
sqe.user_data,
ioUringOpToString(sqe.opcode),
......@@ -89,7 +90,8 @@ void toStream(std::ostream& os, const struct io_uring_sqe& sqe) {
if (i) {
os << ",";
}
os << folly::format(
fmt::print(
os,
"buf={}, offset={}, nbytes={}",
iovec[i].iov_base,
offset,
......
......@@ -95,7 +95,7 @@ const folly::F14FastSet<uint64_t>& getF14Baseline() {
folly::BenchmarkSuspender guard;
static const auto set = [] {
folly::F14FastSet<uint64_t> ret(uniform.begin(), uniform.end());
LOG(INFO) << folly::format(
LOG(INFO) << folly::sformat(
"Built F14FastSet, size: {}, space: {}",
ret.size(),
folly::prettyPrint(
......@@ -112,7 +112,7 @@ const folly::compression::MutableEliasFanoCompressedList& getEFBaseline() {
folly::BenchmarkSuspender guard;
static auto list = [] {
auto ret = EFEncoder::encode(uniform.begin(), uniform.end());
LOG(INFO) << folly::format(
LOG(INFO) << folly::sformat(
"Built Elias-Fano list, space: {}",
folly::prettyPrint(
ret.data.size(), folly::PrettyType::PRETTY_BYTES_IEC));
......@@ -207,7 +207,7 @@ void benchmarkSetup() {
boost::sort::spreadsort::integer_sort(uniform.begin(), uniform.end());
buildQuotientMultiSet(uniform);
LOG(INFO) << folly::format(
LOG(INFO) << folly::sformat(
"Built QuotientMultiSet, space: {}",
folly::prettyPrint(qmsData.size(), folly::PrettyType::PRETTY_BYTES_IEC));
}
......
......@@ -530,23 +530,10 @@ TEST(IOBuf, Format) {
IOBuf head(IOBuf::CREATE, 24);
Appender app(&head, 32);
format("{}", "test")(app);
// Test compatibility with the legacy format API.
app(folly::StringPiece("test"));
EXPECT_EQ(head.length(), 4);
EXPECT_EQ(0, memcmp(head.data(), "test", 4));
auto fmt = format(
"{}{} {}{} {:#x}",
32,
"this string is",
"longer than our original allocation size,",
"and will therefore require a new allocation",
0x12345678);
fmt(app);
EXPECT_EQ(
"test32this string is longer than our original "
"allocation size,and will therefore require a "
"new allocation 0x12345678",
head.moveToFbString().toStdString());
}
TEST(IOBuf, QueueAppender) {
......
......@@ -274,7 +274,7 @@ std::string CustomLogFormatter::formatMessage(
// If the message contains multiple lines, ensure that the log header is
// prepended before each message line.
else {
const auto headerFormatter = folly::format(
const auto header = folly::sformat(
logFormat_,
getGlogLevelName(message.getLevel())[0],
ltime.tm_mon + 1,
......@@ -321,7 +321,7 @@ std::string CustomLogFormatter::formatMessage(
}
auto line = msgData.subpiece(idx, end - idx);
headerFormatter.appendTo(buffer);
buffer += header;
buffer.append(line.data(), line.size());
buffer.push_back('\n');
......
......@@ -59,7 +59,7 @@ std::string GlogStyleFormatter::formatMessage(
}
auto basename = message.getFileBaseName();
auto headerFormatter = folly::format(
auto header = folly::sformat(
"{}{:02d}{:02d} {:02d}:{:02d}:{:02d}.{:06d} {:5d} {}:{}{}] ",
getGlogLevelName(message.getLevel())[0],
ltime.tm_mon + 1,
......@@ -92,9 +92,6 @@ std::string GlogStyleFormatter::formatMessage(
if (message.containsNewlines()) {
// If there are multiple lines in the log message, add a header
// before each one.
std::string header;
header.reserve(headerLengthGuess);
headerFormatter.appendTo(header);
buffer.reserve(
((header.size() + 1) * message.getNumNewlines()) + msgData.size());
......@@ -118,7 +115,7 @@ std::string GlogStyleFormatter::formatMessage(
}
} else {
buffer.reserve(headerLengthGuess + msgData.size());
headerFormatter.appendTo(buffer);
buffer.append(header);
buffer.append(msgData.data(), msgData.size());
buffer.push_back('\n');
}
......
......@@ -21,7 +21,7 @@
#include <thread>
#include <vector>
#include <folly/Format.h>
#include <fmt/ostream.h>
#include <folly/portability/GTest.h>
#include <folly/synchronization/test/Barrier.h>
......@@ -67,8 +67,9 @@ bool operator==(const CtorCounts& lhs, const CtorCounts& rhs) {
}
std::ostream& operator<<(std::ostream& out, const CtorCounts& counts) {
return out << folly::format(
"CtorCounts({}, {}, {})", counts.ctor, counts.copy, counts.move);
fmt::print(
out, "CtorCounts({}, {}, {})", counts.ctor, counts.copy, counts.move);
return out;
}
} // namespace
......
......@@ -20,9 +20,9 @@
#include <cerrno>
#include <utility>
#include <fmt/core.h>
#include <glog/logging.h>
#include <folly/Format.h>
#include <folly/Portability.h>
#include <folly/portability/GFlags.h>
#include <folly/portability/SysMman.h>
......@@ -306,8 +306,7 @@ bool MemoryMapping::mlock(LockMode mode, LockFlags flags) {
return true;
}
auto msg =
folly::format("mlock({}) failed at {}", mapLength_, amountSucceeded);
auto msg = fmt::format("mlock({}) failed at {}", mapLength_, amountSucceeded);
if (mode == LockMode::TRY_LOCK && errno == EPERM) {
PLOG(WARNING) << msg;
} else if (mode == LockMode::TRY_LOCK && errno == ENOMEM) {
......@@ -363,7 +362,7 @@ MemoryMapping::~MemoryMapping() {
size_t(mapLength_),
options_.pageSize,
amountSucceeded)) {
PLOG(FATAL) << folly::format(
PLOG(FATAL) << fmt::format(
"munmap({}) failed at {}", mapLength_, amountSucceeded);
}
}
......
......@@ -27,6 +27,8 @@
#include <folly/init/Init.h>
#include <folly/json.h>
FOLLY_GNU_DISABLE_WARNING("-Wdeprecated-declarations")
using namespace folly;
namespace {
......@@ -162,24 +164,8 @@ BENCHMARK(format_nested_strings, iters) {
format(
&out,
"{} {}",
format("{} {}", i, i + 1).str(),
format("{} {}", -i, -i - 1).str());
});
}
}
}
BENCHMARK_RELATIVE(format_nested_fbstrings, iters) {
BenchmarkSuspender suspender;
while (iters--) {
for (int i = 0; i < 1000; ++i) {
fbstring out;
suspender.dismissing([&] {
format(
&out,
"{} {}",
format("{} {}", i, i + 1).str(),
format("{} {}", -i, -i - 1).str());
sformat("{} {}", i, i + 1),
sformat("{} {}", -i, -i - 1));
});
}
}
......@@ -314,7 +300,6 @@ BENCHMARK_RELATIVE(sformat_long_string_safe, iters) {
// bigFormat_format 90.41% 196.91us 5.08K
// ----------------------------------------------------------------------------
// format_nested_strings 317.65us 3.15K
// format_nested_fbstrings 99.89% 318.01us 3.14K
// format_nested_direct 116.52% 272.62us 3.67K
// ----------------------------------------------------------------------------
// copy_short_string 28.33ns 35.30M
......
......@@ -28,34 +28,6 @@
using namespace folly;
TEST(FormatOther, file) {
// Test writing to FILE. I'd use open_memstream but that's not available
// outside of Linux (even though it's in POSIX.1-2008).
{
int fds[2];
CHECK_ERR(pipe(fds));
SCOPE_EXIT {
// fclose on Windows automatically closes the underlying
// file descriptor.
if (!kIsWindows) {
closeNoInt(fds[1]);
}
};
{
FILE* fp = fdopen(fds[1], "wb");
PCHECK(fp);
SCOPE_EXIT { fclose(fp); };
writeTo(fp, format("{} {}", 42, 23)); // <= 512 bytes (PIPE_BUF)
}
char buf[512];
ssize_t n = readFull(fds[0], buf, sizeof(buf));
CHECK_GE(n, 0);
EXPECT_EQ("42 23", std::string(buf, n));
}
}
TEST(FormatOther, dynamic) {
auto dyn = parseJson(
"{\n"
......
......@@ -21,6 +21,8 @@
#include <folly/Utility.h>
#include <folly/portability/GTest.h>
FOLLY_GNU_DISABLE_WARNING("-Wdeprecated")
using namespace folly;
template <class Uint>
......
......@@ -135,7 +135,7 @@ void follyFmtOutputSize(int iters, int param) {
BENCHMARK_SUSPEND { buffer.resize(param, 'x'); }
for (int64_t i = 0; i < iters; ++i) {
string s = format("msg: {}, {}, {}", 10, 20, buffer).str();
string s = sformat("msg: {}, {}, {}", 10, 20, buffer);
}
}
......@@ -149,19 +149,6 @@ BENCHMARK_PARAM(follyFmtOutputSize, 64)
BENCHMARK_PARAM(follyFmtOutputSize, 256)
BENCHMARK_PARAM(follyFmtOutputSize, 1024)
// Benchmark simple fmt append behavior; intended as a comparison
// against stringAppendf.
BENCHMARK(follyFmtAppendfBenchmark, iters) {
for (size_t i = 0; i < iters; ++i) {
string s;
BENCHMARK_SUSPEND { s.reserve(kAppendBufSize); }
for (size_t j = 0; j < kAppendBufSize; ++j) {
format("{}", 1).appendTo(s);
}
DCHECK_EQ(s.size(), kAppendBufSize);
}
}
namespace {
fbstring cbmString;
fbstring cbmEscapedString;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment