Commit e1a2cdef authored by Adam Simpkins's avatar Adam Simpkins Committed by Sara Golemon

revert format()'s behavior of crashing on error

Summary:
This reverts 61e20daa, which changed the format code to abort on error.

I do plan to restore the crashing behavior, but I plan to make it
optional.  I will add a formatSafe() function that throws on error,
while format() catches these exceptions and crashes.

This diff is an intermediate diff to make it easier to review the
changes.  This is close to a straight revert of 61e20daa and 61a41c9b.
However, I did leave the new test case, as well as the
FormatArg::errorStr() method added by those diffs.

Test Plan:
Verified that the existing format tests pass.

Also added a handful of new tests for bad format strings.  This did
catch one bug in the code where it wasn't correctly checking for a null
return value from memchr().

Reviewed By: delong.j@fb.com

FB internal diff: D1144298
parent e0b08360
...@@ -169,7 +169,10 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -169,7 +169,10 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
out(StringPiece(p, q)); out(StringPiece(p, q));
p = q; p = q;
CHECK(p != end && *p == '}') << "single '}' in format string"; if (p == end || *p != '}') {
throw std::invalid_argument(
"folly::format: single '}' in format string");
}
++p; ++p;
} }
}; };
...@@ -186,7 +189,10 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -186,7 +189,10 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
outputString(StringPiece(p, q)); outputString(StringPiece(p, q));
p = q + 1; p = q + 1;
CHECK(p != end) << "'{' at end of format string"; if (p == end) {
throw std::invalid_argument(
"folly::format: '}' at end of format string");
}
// "{{" -> "{" // "{{" -> "{"
if (*p == '{') { if (*p == '{') {
...@@ -197,7 +203,9 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -197,7 +203,9 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
// Format string // Format string
q = static_cast<const char*>(memchr(p, '}', end - p)); q = static_cast<const char*>(memchr(p, '}', end - p));
CHECK(q != end) << "missing ending '}'"; if (q == nullptr) {
throw std::invalid_argument("folly::format: missing ending '}'");
}
FormatArg arg(StringPiece(p, q)); FormatArg arg(StringPiece(p, q));
p = q + 1; p = q + 1;
...@@ -219,16 +227,17 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const { ...@@ -219,16 +227,17 @@ void Formatter<containerMode, Args...>::operator()(Output& out) const {
try { try {
argIndex = to<int>(piece); argIndex = to<int>(piece);
} catch (const std::out_of_range& e) { } catch (const std::out_of_range& e) {
LOG(FATAL) << "argument index must be integer"; arg.error("argument index must be integer");
} }
CHECK(argIndex >= 0) arg.enforce(argIndex >= 0, "argument index must be non-negative");
<< arg.errorStr("argument index must be non-negative");
hasExplicitArgIndex = true; hasExplicitArgIndex = true;
} }
} }
CHECK(!hasDefaultArgIndex || !hasExplicitArgIndex) if (hasDefaultArgIndex && hasExplicitArgIndex) {
<< "may not have both default and explicit arg indexes"; throw std::invalid_argument(
"folly::format: may not have both default and explicit arg indexes");
}
doFormat(argIndex, arg, out); doFormat(argIndex, arg, out);
} }
...@@ -403,8 +412,8 @@ class FormatValue< ...@@ -403,8 +412,8 @@ class FormatValue<
uval = val_; uval = val_;
sign = '\0'; sign = '\0';
CHECK(arg.sign == FormatArg::Sign::DEFAULT) arg.enforce(arg.sign == FormatArg::Sign::DEFAULT,
<< arg.errorStr("sign specifications not allowed for unsigned values"); "sign specifications not allowed for unsigned values");
} }
// max of: // max of:
...@@ -431,9 +440,9 @@ class FormatValue< ...@@ -431,9 +440,9 @@ class FormatValue<
switch (presentation) { switch (presentation) {
case 'n': // TODO(tudorb): locale awareness? case 'n': // TODO(tudorb): locale awareness?
case 'd': case 'd':
CHECK(!arg.basePrefix) arg.enforce(!arg.basePrefix,
<< arg.errorStr("base prefix not allowed with '", presentation, "base prefix not allowed with '", presentation,
"' specifier"); "' specifier");
if (arg.thousandsSeparator) { if (arg.thousandsSeparator) {
useSprintf("%'ju"); useSprintf("%'ju");
} else { } else {
...@@ -443,21 +452,21 @@ class FormatValue< ...@@ -443,21 +452,21 @@ class FormatValue<
} }
break; break;
case 'c': case 'c':
CHECK(!arg.basePrefix) arg.enforce(!arg.basePrefix,
<< arg.errorStr("base prefix not allowed with '", presentation, "base prefix not allowed with '", presentation,
"' specifier"); "' specifier");
CHECK(!arg.thousandsSeparator) arg.enforce(!arg.thousandsSeparator,
<< arg.errorStr("thousands separator (',') not allowed with '", "thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufBegin = valBuf + 3; valBufBegin = valBuf + 3;
*valBufBegin = static_cast<char>(uval); *valBufBegin = static_cast<char>(uval);
valBufEnd = valBufBegin + 1; valBufEnd = valBufBegin + 1;
break; break;
case 'o': case 'o':
case 'O': case 'O':
CHECK(!arg.thousandsSeparator) arg.enforce(!arg.thousandsSeparator,
<< arg.errorStr("thousands separator (',') not allowed with '", "thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval); valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval);
if (arg.basePrefix) { if (arg.basePrefix) {
...@@ -466,9 +475,9 @@ class FormatValue< ...@@ -466,9 +475,9 @@ class FormatValue<
} }
break; break;
case 'x': case 'x':
CHECK(!arg.thousandsSeparator) arg.enforce(!arg.thousandsSeparator,
<< arg.errorStr("thousands separator (',') not allowed with '", "thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1,
uval); uval);
...@@ -479,9 +488,9 @@ class FormatValue< ...@@ -479,9 +488,9 @@ class FormatValue<
} }
break; break;
case 'X': case 'X':
CHECK(!arg.thousandsSeparator) arg.enforce(!arg.thousandsSeparator,
<< arg.errorStr("thousands separator (',') not allowed with '", "thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1,
uval); uval);
...@@ -493,9 +502,9 @@ class FormatValue< ...@@ -493,9 +502,9 @@ class FormatValue<
break; break;
case 'b': case 'b':
case 'B': case 'B':
CHECK(!arg.thousandsSeparator) arg.enforce(!arg.thousandsSeparator,
<< arg.errorStr("thousands separator (',') not allowed with '", "thousands separator (',') not allowed with '",
presentation, "' specifier"); presentation, "' specifier");
valBufEnd = valBuf + valBufSize - 1; valBufEnd = valBuf + valBufSize - 1;
valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1, valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1,
uval); uval);
...@@ -506,7 +515,7 @@ class FormatValue< ...@@ -506,7 +515,7 @@ class FormatValue<
} }
break; break;
default: default:
LOG(FATAL) << arg.errorStr("invalid specifier '", presentation, "'"); arg.error("invalid specifier '", presentation, "'");
} }
if (sign) { if (sign) {
...@@ -624,7 +633,7 @@ class FormatValue<double> { ...@@ -624,7 +633,7 @@ class FormatValue<double> {
exponentSymbol, exponentSymbol,
-4, arg.precision, -4, arg.precision,
0, 0); 0, 0);
CHECK(conv.ToExponential(val, arg.precision, &builder)); arg.enforce(conv.ToExponential(val, arg.precision, &builder));
} }
break; break;
case 'n': // should be locale-aware, but isn't case 'n': // should be locale-aware, but isn't
...@@ -644,11 +653,11 @@ class FormatValue<double> { ...@@ -644,11 +653,11 @@ class FormatValue<double> {
exponentSymbol, exponentSymbol,
-4, arg.precision, -4, arg.precision,
0, 0); 0, 0);
CHECK(conv.ToShortest(val, &builder)); arg.enforce(conv.ToShortest(val, &builder));
} }
break; break;
default: default:
LOG(FATAL) << arg.errorStr("invalid specifier '", arg.presentation, "'"); arg.error("invalid specifier '", arg.presentation, "'");
} }
int len = builder.position(); int len = builder.position();
...@@ -705,9 +714,9 @@ class FormatValue< ...@@ -705,9 +714,9 @@ class FormatValue<
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
if (arg.keyEmpty()) { if (arg.keyEmpty()) {
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
CHECK(arg.presentation == FormatArg::kDefaultPresentation || arg.enforce(arg.presentation == FormatArg::kDefaultPresentation ||
arg.presentation == 's') arg.presentation == 's',
<< arg.errorStr("invalid specifier '", arg.presentation, "'"); "invalid specifier '", arg.presentation, "'");
format_value::formatString(val_, arg, cb); format_value::formatString(val_, arg, cb);
} else { } else {
FormatValue<char>(val_.at(arg.splitIntKey())).format(arg, cb); FormatValue<char>(val_.at(arg.splitIntKey())).format(arg, cb);
...@@ -727,8 +736,8 @@ class FormatValue<std::nullptr_t> { ...@@ -727,8 +736,8 @@ class FormatValue<std::nullptr_t> {
template <class FormatCallback> template <class FormatCallback>
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
CHECK(arg.presentation == FormatArg::kDefaultPresentation) arg.enforce(arg.presentation == FormatArg::kDefaultPresentation,
<< arg.errorStr("invalid specifier '", arg.presentation, "'"); "invalid specifier '", arg.presentation, "'");
format_value::formatString("(null)", arg, cb); format_value::formatString("(null)", arg, cb);
} }
}; };
...@@ -778,8 +787,8 @@ class FormatValue< ...@@ -778,8 +787,8 @@ class FormatValue<
} else { } else {
// Print as a pointer, in hex. // Print as a pointer, in hex.
arg.validate(FormatArg::Type::OTHER); arg.validate(FormatArg::Type::OTHER);
CHECK(arg.presentation == FormatArg::kDefaultPresentation) arg.enforce(arg.presentation == FormatArg::kDefaultPresentation,
<< arg.errorStr("invalid specifier '", arg.presentation, "'"); "invalid specifier '", arg.presentation, "'");
arg.basePrefix = true; arg.basePrefix = true;
arg.presentation = 'x'; arg.presentation = 'x';
if (arg.align == FormatArg::Align::DEFAULT) { if (arg.align == FormatArg::Align::DEFAULT) {
...@@ -799,7 +808,7 @@ class TryFormatValue { ...@@ -799,7 +808,7 @@ class TryFormatValue {
public: public:
template <class FormatCallback> template <class FormatCallback>
static void formatOrFail(T& value, FormatArg& arg, FormatCallback& cb) { static void formatOrFail(T& value, FormatArg& arg, FormatCallback& cb) {
LOG(FATAL) << arg.errorStr("No formatter available for this type"); arg.error("No formatter available for this type");
} }
}; };
...@@ -1040,7 +1049,7 @@ class FormatValue<std::pair<A, B>> { ...@@ -1040,7 +1049,7 @@ class FormatValue<std::pair<A, B>> {
FormatValue<typename std::decay<B>::type>(val_.second).format(arg, cb); FormatValue<typename std::decay<B>::type>(val_.second).format(arg, cb);
break; break;
default: default:
LOG(FATAL) << arg.errorStr("invalid index for pair"); arg.error("invalid index for pair");
} }
} }
...@@ -1058,7 +1067,7 @@ class FormatValue<std::tuple<Args...>> { ...@@ -1058,7 +1067,7 @@ class FormatValue<std::tuple<Args...>> {
template <class FormatCallback> template <class FormatCallback>
void format(FormatArg& arg, FormatCallback& cb) const { void format(FormatArg& arg, FormatCallback& cb) const {
int key = arg.splitIntKey(); int key = arg.splitIntKey();
CHECK(key >= 0) << arg.errorStr("tuple index must be non-negative"); arg.enforce(key >= 0, "tuple index must be non-negative");
doFormat(key, arg, cb); doFormat(key, arg, cb);
} }
...@@ -1068,7 +1077,7 @@ class FormatValue<std::tuple<Args...>> { ...@@ -1068,7 +1077,7 @@ class FormatValue<std::tuple<Args...>> {
template <size_t K, class Callback> template <size_t K, class Callback>
typename std::enable_if<K == valueCount>::type typename std::enable_if<K == valueCount>::type
doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const { doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
LOG(FATAL) << arg.errorStr("tuple index out of range, max=", i); arg.enforce("tuple index out of range, max=", i);
} }
template <size_t K, class Callback> template <size_t K, class Callback>
......
/* /*
* Copyright 2013 Facebook, Inc. * Copyright 2014 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
...@@ -70,7 +70,7 @@ void FormatArg::initSlow() { ...@@ -70,7 +70,7 @@ void FormatArg::initSlow() {
} }
if (*p == '0') { if (*p == '0') {
CHECK(align == Align::DEFAULT) << errorStr("alignment specified twice"); enforce(align == Align::DEFAULT, "alignment specified twice");
fill = '0'; fill = '0';
align = Align::PAD_AFTER_SIGN; align = Align::PAD_AFTER_SIGN;
if (++p == end) return; if (++p == end) return;
...@@ -105,31 +105,31 @@ void FormatArg::initSlow() { ...@@ -105,31 +105,31 @@ void FormatArg::initSlow() {
if (++p == end) return; if (++p == end) return;
} }
LOG(FATAL) << "extra characters in format string"; error("extra characters in format string");
} }
void FormatArg::validate(Type type) const { void FormatArg::validate(Type type) const {
CHECK(keyEmpty()) << "index not allowed"; enforce(keyEmpty(), "index not allowed");
switch (type) { switch (type) {
case Type::INTEGER: case Type::INTEGER:
CHECK(precision == kDefaultPrecision) enforce(precision == kDefaultPrecision,
<< errorStr("precision not allowed on integers"); "precision not allowed on integers");
break; break;
case Type::FLOAT: case Type::FLOAT:
CHECK(!basePrefix) enforce(!basePrefix,
<< errorStr("base prefix ('#') specifier only allowed on integers"); "base prefix ('#') specifier only allowed on integers");
CHECK(!thousandsSeparator) enforce(!thousandsSeparator,
<< errorStr("thousands separator (',') only allowed on integers"); "thousands separator (',') only allowed on integers");
break; break;
case Type::OTHER: case Type::OTHER:
CHECK(align != Align::PAD_AFTER_SIGN) enforce(align != Align::PAD_AFTER_SIGN,
<< errorStr("'='alignment only allowed on numbers"); "'='alignment only allowed on numbers");
CHECK(sign == Sign::DEFAULT) enforce(sign == Sign::DEFAULT,
<< errorStr("sign specifier only allowed on numbers"); "sign specifier only allowed on numbers");
CHECK(!basePrefix) enforce(!basePrefix,
<< errorStr("base prefix ('#') specifier only allowed on integers"); "base prefix ('#') specifier only allowed on integers");
CHECK(!thousandsSeparator) enforce(!thousandsSeparator,
<< errorStr("thousands separator (',') only allowed on integers"); "thousands separator (',') only allowed on integers");
break; break;
} }
} }
......
...@@ -122,7 +122,7 @@ class Formatter { ...@@ -122,7 +122,7 @@ class Formatter {
template <size_t K, class Callback> template <size_t K, class Callback>
typename std::enable_if<K == valueCount>::type typename std::enable_if<K == valueCount>::type
doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const { doFormatFrom(size_t i, FormatArg& arg, Callback& cb) const {
LOG(FATAL) << arg.errorStr("argument index out of range, max=", i); arg.error("argument index out of range, max=", i);
} }
template <size_t K, class Callback> template <size_t K, class Callback>
......
/* /*
* Copyright 2013 Facebook, Inc. * Copyright 2014 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
...@@ -202,7 +202,7 @@ inline void FormatArg::error(Args&&... args) const { ...@@ -202,7 +202,7 @@ inline void FormatArg::error(Args&&... args) const {
template <bool emptyOk> template <bool emptyOk>
inline StringPiece FormatArg::splitKey() { inline StringPiece FormatArg::splitKey() {
CHECK(nextKeyMode_ != NextKeyMode::INT) << errorStr("integer key expected"); enforce(nextKeyMode_ != NextKeyMode::INT, "integer key expected");
return doSplitKey<emptyOk>(); return doSplitKey<emptyOk>();
} }
...@@ -210,12 +210,16 @@ template <bool emptyOk> ...@@ -210,12 +210,16 @@ template <bool emptyOk>
inline StringPiece FormatArg::doSplitKey() { inline StringPiece FormatArg::doSplitKey() {
if (nextKeyMode_ == NextKeyMode::STRING) { if (nextKeyMode_ == NextKeyMode::STRING) {
nextKeyMode_ = NextKeyMode::NONE; nextKeyMode_ = NextKeyMode::NONE;
CHECK(emptyOk || !nextKey_.empty()) << errorStr("non-empty key required"); if (!emptyOk) { // static
enforce(!nextKey_.empty(), "non-empty key required");
}
return nextKey_; return nextKey_;
} }
if (key_.empty()) { if (key_.empty()) {
CHECK(emptyOk) << errorStr("non-empty key required"); if (!emptyOk) { // static
error("non-empty key required");
}
return StringPiece(); return StringPiece();
} }
...@@ -225,7 +229,7 @@ inline StringPiece FormatArg::doSplitKey() { ...@@ -225,7 +229,7 @@ inline StringPiece FormatArg::doSplitKey() {
if (e[-1] == ']') { if (e[-1] == ']') {
--e; --e;
p = static_cast<const char*>(memchr(b, '[', e - b)); p = static_cast<const char*>(memchr(b, '[', e - b));
CHECK(p) << errorStr("unmatched ']'"); enforce(p, "unmatched ']'");
} else { } else {
p = static_cast<const char*>(memchr(b, '.', e - b)); p = static_cast<const char*>(memchr(b, '.', e - b));
} }
...@@ -235,8 +239,9 @@ inline StringPiece FormatArg::doSplitKey() { ...@@ -235,8 +239,9 @@ inline StringPiece FormatArg::doSplitKey() {
p = e; p = e;
key_.clear(); key_.clear();
} }
CHECK(emptyOk || b != p) << errorStr("non-empty key required"); if (!emptyOk) { // static
enforce(b != p, "non-empty key required");
}
return StringPiece(b, p); return StringPiece(b, p);
} }
...@@ -248,7 +253,7 @@ inline int FormatArg::splitIntKey() { ...@@ -248,7 +253,7 @@ inline int FormatArg::splitIntKey() {
try { try {
return to<int>(doSplitKey<true>()); return to<int>(doSplitKey<true>());
} catch (const std::out_of_range& e) { } catch (const std::out_of_range& e) {
LOG(FATAL) << errorStr("integer key required"); error("integer key required");
return 0; // unreached return 0; // unreached
} }
} }
......
/* /*
* Copyright 2013 Facebook, Inc. * Copyright 2014 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
...@@ -296,6 +296,20 @@ TEST(Format, Custom) { ...@@ -296,6 +296,20 @@ TEST(Format, Custom) {
EXPECT_NE("", fstr("{}", &kv)); EXPECT_NE("", fstr("{}", &kv));
} }
namespace {
struct Opaque {
int k;
};
} // namespace
TEST(Format, Unformatted) {
Opaque o;
EXPECT_NE("", fstr("{}", &o));
EXPECT_THROW(fstr("{0[0]}", &o), std::invalid_argument);
}
TEST(Format, Nested) { TEST(Format, Nested) {
EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4))); EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4)));
// //
...@@ -312,6 +326,18 @@ TEST(Format, OutOfBounds) { ...@@ -312,6 +326,18 @@ TEST(Format, OutOfBounds) {
std::map<std::string, int> map{{"hello", 0}, {"world", 1}}; std::map<std::string, int> map{{"hello", 0}, {"world", 1}};
EXPECT_EQ("hello = 0", fstr("hello = {[hello]}", map)); EXPECT_EQ("hello = 0", fstr("hello = {[hello]}", map));
EXPECT_THROW(fstr("{[nope]}", map), std::out_of_range); EXPECT_THROW(fstr("{[nope]}", map), std::out_of_range);
EXPECT_THROW(vstr("{nope}", map), std::out_of_range);
}
TEST(Format, BogusFormatString) {
EXPECT_THROW(fstr("}"), std::invalid_argument);
EXPECT_THROW(fstr("foo}bar"), std::invalid_argument);
EXPECT_THROW(fstr("foo{bar"), std::invalid_argument);
EXPECT_THROW(fstr("{[test]"), std::invalid_argument);
// This one fails in detail::enforceWhitespace(), which throws
// std::range_error
EXPECT_THROW(fstr("{0[test}"), std::exception);
} }
int main(int argc, char *argv[]) { int main(int argc, char *argv[]) {
......
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