From e1a2cdefda74db3039afc03e9579e7cf56b910cd Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Sun, 26 Jan 2014 17:47:48 -0800 Subject: [PATCH] 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 --- folly/Format-inl.h | 101 +++++++++++++++++++++----------------- folly/Format.cpp | 36 +++++++------- folly/Format.h | 2 +- folly/FormatArg.h | 21 +++++--- folly/test/FormatTest.cpp | 28 ++++++++++- 5 files changed, 114 insertions(+), 74 deletions(-) diff --git a/folly/Format-inl.h b/folly/Format-inl.h index 8f4bb8e9..f33ff62b 100644 --- a/folly/Format-inl.h +++ b/folly/Format-inl.h @@ -169,7 +169,10 @@ void Formatter::operator()(Output& out) const { out(StringPiece(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; } }; @@ -186,7 +189,10 @@ void Formatter::operator()(Output& out) const { outputString(StringPiece(p, q)); 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 == '{') { @@ -197,7 +203,9 @@ void Formatter::operator()(Output& out) const { // Format string q = static_cast(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)); p = q + 1; @@ -219,16 +227,17 @@ void Formatter::operator()(Output& out) const { try { argIndex = to(piece); } 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.errorStr("argument index must be non-negative"); + arg.enforce(argIndex >= 0, "argument index must be non-negative"); hasExplicitArgIndex = true; } } - CHECK(!hasDefaultArgIndex || !hasExplicitArgIndex) - << "may not have both default and explicit arg indexes"; + if (hasDefaultArgIndex && hasExplicitArgIndex) { + throw std::invalid_argument( + "folly::format: may not have both default and explicit arg indexes"); + } doFormat(argIndex, arg, out); } @@ -403,8 +412,8 @@ class FormatValue< uval = val_; sign = '\0'; - CHECK(arg.sign == FormatArg::Sign::DEFAULT) - << arg.errorStr("sign specifications not allowed for unsigned values"); + arg.enforce(arg.sign == FormatArg::Sign::DEFAULT, + "sign specifications not allowed for unsigned values"); } // max of: @@ -431,9 +440,9 @@ class FormatValue< switch (presentation) { case 'n': // TODO(tudorb): locale awareness? case 'd': - CHECK(!arg.basePrefix) - << arg.errorStr("base prefix not allowed with '", presentation, - "' specifier"); + arg.enforce(!arg.basePrefix, + "base prefix not allowed with '", presentation, + "' specifier"); if (arg.thousandsSeparator) { useSprintf("%'ju"); } else { @@ -443,21 +452,21 @@ class FormatValue< } break; case 'c': - CHECK(!arg.basePrefix) - << arg.errorStr("base prefix not allowed with '", presentation, - "' specifier"); - CHECK(!arg.thousandsSeparator) - << arg.errorStr("thousands separator (',') not allowed with '", - presentation, "' specifier"); + arg.enforce(!arg.basePrefix, + "base prefix not allowed with '", presentation, + "' specifier"); + arg.enforce(!arg.thousandsSeparator, + "thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufBegin = valBuf + 3; *valBufBegin = static_cast(uval); valBufEnd = valBufBegin + 1; break; case 'o': case 'O': - CHECK(!arg.thousandsSeparator) - << arg.errorStr("thousands separator (',') not allowed with '", - presentation, "' specifier"); + arg.enforce(!arg.thousandsSeparator, + "thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToOctal(valBuf, valBufSize - 1, uval); if (arg.basePrefix) { @@ -466,9 +475,9 @@ class FormatValue< } break; case 'x': - CHECK(!arg.thousandsSeparator) - << arg.errorStr("thousands separator (',') not allowed with '", - presentation, "' specifier"); + arg.enforce(!arg.thousandsSeparator, + "thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToHexLower(valBuf, valBufSize - 1, uval); @@ -479,9 +488,9 @@ class FormatValue< } break; case 'X': - CHECK(!arg.thousandsSeparator) - << arg.errorStr("thousands separator (',') not allowed with '", - presentation, "' specifier"); + arg.enforce(!arg.thousandsSeparator, + "thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToHexUpper(valBuf, valBufSize - 1, uval); @@ -493,9 +502,9 @@ class FormatValue< break; case 'b': case 'B': - CHECK(!arg.thousandsSeparator) - << arg.errorStr("thousands separator (',') not allowed with '", - presentation, "' specifier"); + arg.enforce(!arg.thousandsSeparator, + "thousands separator (',') not allowed with '", + presentation, "' specifier"); valBufEnd = valBuf + valBufSize - 1; valBufBegin = valBuf + detail::uintToBinary(valBuf, valBufSize - 1, uval); @@ -506,7 +515,7 @@ class FormatValue< } break; default: - LOG(FATAL) << arg.errorStr("invalid specifier '", presentation, "'"); + arg.error("invalid specifier '", presentation, "'"); } if (sign) { @@ -624,7 +633,7 @@ class FormatValue { exponentSymbol, -4, arg.precision, 0, 0); - CHECK(conv.ToExponential(val, arg.precision, &builder)); + arg.enforce(conv.ToExponential(val, arg.precision, &builder)); } break; case 'n': // should be locale-aware, but isn't @@ -644,11 +653,11 @@ class FormatValue { exponentSymbol, -4, arg.precision, 0, 0); - CHECK(conv.ToShortest(val, &builder)); + arg.enforce(conv.ToShortest(val, &builder)); } break; default: - LOG(FATAL) << arg.errorStr("invalid specifier '", arg.presentation, "'"); + arg.error("invalid specifier '", arg.presentation, "'"); } int len = builder.position(); @@ -705,9 +714,9 @@ class FormatValue< void format(FormatArg& arg, FormatCallback& cb) const { if (arg.keyEmpty()) { arg.validate(FormatArg::Type::OTHER); - CHECK(arg.presentation == FormatArg::kDefaultPresentation || - arg.presentation == 's') - << arg.errorStr("invalid specifier '", arg.presentation, "'"); + arg.enforce(arg.presentation == FormatArg::kDefaultPresentation || + arg.presentation == 's', + "invalid specifier '", arg.presentation, "'"); format_value::formatString(val_, arg, cb); } else { FormatValue(val_.at(arg.splitIntKey())).format(arg, cb); @@ -727,8 +736,8 @@ class FormatValue { template void format(FormatArg& arg, FormatCallback& cb) const { arg.validate(FormatArg::Type::OTHER); - CHECK(arg.presentation == FormatArg::kDefaultPresentation) - << arg.errorStr("invalid specifier '", arg.presentation, "'"); + arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, + "invalid specifier '", arg.presentation, "'"); format_value::formatString("(null)", arg, cb); } }; @@ -778,8 +787,8 @@ class FormatValue< } else { // Print as a pointer, in hex. arg.validate(FormatArg::Type::OTHER); - CHECK(arg.presentation == FormatArg::kDefaultPresentation) - << arg.errorStr("invalid specifier '", arg.presentation, "'"); + arg.enforce(arg.presentation == FormatArg::kDefaultPresentation, + "invalid specifier '", arg.presentation, "'"); arg.basePrefix = true; arg.presentation = 'x'; if (arg.align == FormatArg::Align::DEFAULT) { @@ -799,7 +808,7 @@ class TryFormatValue { public: template 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> { FormatValue::type>(val_.second).format(arg, cb); break; default: - LOG(FATAL) << arg.errorStr("invalid index for pair"); + arg.error("invalid index for pair"); } } @@ -1058,7 +1067,7 @@ class FormatValue> { template void format(FormatArg& arg, FormatCallback& cb) const { 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); } @@ -1068,7 +1077,7 @@ class FormatValue> { template typename std::enable_if::type 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 diff --git a/folly/Format.cpp b/folly/Format.cpp index 169eca08..553c550a 100644 --- a/folly/Format.cpp +++ b/folly/Format.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2013 Facebook, Inc. + * Copyright 2014 Facebook, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -70,7 +70,7 @@ void FormatArg::initSlow() { } if (*p == '0') { - CHECK(align == Align::DEFAULT) << errorStr("alignment specified twice"); + enforce(align == Align::DEFAULT, "alignment specified twice"); fill = '0'; align = Align::PAD_AFTER_SIGN; if (++p == end) return; @@ -105,31 +105,31 @@ void FormatArg::initSlow() { if (++p == end) return; } - LOG(FATAL) << "extra characters in format string"; + error("extra characters in format string"); } void FormatArg::validate(Type type) const { - CHECK(keyEmpty()) << "index not allowed"; + enforce(keyEmpty(), "index not allowed"); switch (type) { case Type::INTEGER: - CHECK(precision == kDefaultPrecision) - << errorStr("precision not allowed on integers"); + enforce(precision == kDefaultPrecision, + "precision not allowed on integers"); break; case Type::FLOAT: - CHECK(!basePrefix) - << errorStr("base prefix ('#') specifier only allowed on integers"); - CHECK(!thousandsSeparator) - << errorStr("thousands separator (',') only allowed on integers"); + enforce(!basePrefix, + "base prefix ('#') specifier only allowed on integers"); + enforce(!thousandsSeparator, + "thousands separator (',') only allowed on integers"); break; case Type::OTHER: - CHECK(align != Align::PAD_AFTER_SIGN) - << errorStr("'='alignment only allowed on numbers"); - CHECK(sign == Sign::DEFAULT) - << errorStr("sign specifier only allowed on numbers"); - CHECK(!basePrefix) - << errorStr("base prefix ('#') specifier only allowed on integers"); - CHECK(!thousandsSeparator) - << errorStr("thousands separator (',') only allowed on integers"); + enforce(align != Align::PAD_AFTER_SIGN, + "'='alignment only allowed on numbers"); + enforce(sign == Sign::DEFAULT, + "sign specifier only allowed on numbers"); + enforce(!basePrefix, + "base prefix ('#') specifier only allowed on integers"); + enforce(!thousandsSeparator, + "thousands separator (',') only allowed on integers"); break; } } diff --git a/folly/Format.h b/folly/Format.h index 589bcdff..da9b83f8 100644 --- a/folly/Format.h +++ b/folly/Format.h @@ -122,7 +122,7 @@ class Formatter { template typename std::enable_if::type 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 diff --git a/folly/FormatArg.h b/folly/FormatArg.h index d48aad7a..4099c130 100644 --- a/folly/FormatArg.h +++ b/folly/FormatArg.h @@ -1,5 +1,5 @@ /* - * Copyright 2013 Facebook, Inc. + * Copyright 2014 Facebook, Inc. * * Licensed under the Apache License, Version 2.0 (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 { template inline StringPiece FormatArg::splitKey() { - CHECK(nextKeyMode_ != NextKeyMode::INT) << errorStr("integer key expected"); + enforce(nextKeyMode_ != NextKeyMode::INT, "integer key expected"); return doSplitKey(); } @@ -210,12 +210,16 @@ template inline StringPiece FormatArg::doSplitKey() { if (nextKeyMode_ == NextKeyMode::STRING) { nextKeyMode_ = NextKeyMode::NONE; - CHECK(emptyOk || !nextKey_.empty()) << errorStr("non-empty key required"); + if (!emptyOk) { // static + enforce(!nextKey_.empty(), "non-empty key required"); + } return nextKey_; } if (key_.empty()) { - CHECK(emptyOk) << errorStr("non-empty key required"); + if (!emptyOk) { // static + error("non-empty key required"); + } return StringPiece(); } @@ -225,7 +229,7 @@ inline StringPiece FormatArg::doSplitKey() { if (e[-1] == ']') { --e; p = static_cast(memchr(b, '[', e - b)); - CHECK(p) << errorStr("unmatched ']'"); + enforce(p, "unmatched ']'"); } else { p = static_cast(memchr(b, '.', e - b)); } @@ -235,8 +239,9 @@ inline StringPiece FormatArg::doSplitKey() { p = e; 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); } @@ -248,7 +253,7 @@ inline int FormatArg::splitIntKey() { try { return to(doSplitKey()); } catch (const std::out_of_range& e) { - LOG(FATAL) << errorStr("integer key required"); + error("integer key required"); return 0; // unreached } } diff --git a/folly/test/FormatTest.cpp b/folly/test/FormatTest.cpp index e81266ea..49a07949 100644 --- a/folly/test/FormatTest.cpp +++ b/folly/test/FormatTest.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2013 Facebook, Inc. + * Copyright 2014 Facebook, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -296,6 +296,20 @@ TEST(Format, Custom) { 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) { EXPECT_EQ("1 2 3 4", fstr("{} {} {}", 1, 2, format("{} {}", 3, 4))); // @@ -312,6 +326,18 @@ TEST(Format, OutOfBounds) { std::map map{{"hello", 0}, {"world", 1}}; EXPECT_EQ("hello = 0", fstr("hello = {[hello]}", map)); 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[]) { -- 2.34.1