Revert "[Support] Validate number of arguments passed to formatv()" #106589
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joker-eph
added
the
skip-precommit-approval
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-mlir Author: Mehdi Amini (joker-eph) ChangesReverts llvm/llvm-project#105745 Some bots are broken apparently. Patch is 23.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106589.diff 7 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 4f30b2a0e7e7da..8f4bd17afc8581 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,10 +1401,7 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
ErrnoNote =
llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
} else {
- // Disable formatv() validation as the case note may not always have the
- // {0} placeholder for function name.
- CaseNote =
- llvm::formatv(false, Case.getNote().str().c_str(), FunctionName);
+ CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
}
const SVal RV = Call.getReturnValue();
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index e3366e6f3ffe19..713d4ccd3c5975 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -5,4 +5,3 @@ set(LLVM_LINK_COMPONENTS
add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
-add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/FormatVariadicBM.cpp b/llvm/benchmarks/FormatVariadicBM.cpp
deleted file mode 100644
index c03ead400d0d5c..00000000000000
--- a/llvm/benchmarks/FormatVariadicBM.cpp
+++ /dev/null
@@ -1,63 +0,0 @@
-//===- FormatVariadicBM.cpp - formatv() benchmark ---------- --------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "benchmark/benchmark.h"
-#include "llvm/Support/FormatVariadic.h"
-#include <algorithm>
-#include <string>
-#include <vector>
-
-using namespace llvm;
-using namespace std;
-
-// Generate a list of format strings that have `NumReplacements` replacements
-// by permuting the replacements and some literal text.
-static vector<string> getFormatStrings(int NumReplacements) {
- vector<string> Components;
- for (int I = 0; I < NumReplacements; I++)
- Components.push_back("{" + to_string(I) + "}");
- // Intersperse these with some other literal text (_).
- const string_view Literal = "____";
- for (char C : Literal)
- Components.push_back(string(1, C));
-
- vector<string> Formats;
- do {
- string Concat;
- for (const string &C : Components)
- Concat += C;
- Formats.emplace_back(Concat);
- } while (next_permutation(Components.begin(), Components.end()));
- return Formats;
-}
-
-// Generate the set of formats to exercise outside the benchmark code.
-static const vector<vector<string>> Formats = {
- getFormatStrings(1), getFormatStrings(2), getFormatStrings(3),
- getFormatStrings(4), getFormatStrings(5),
-};
-
-// Benchmark formatv() for a variety of format strings and 1-5 replacements.
-static void BM_FormatVariadic(benchmark::State &state) {
- for (auto _ : state) {
- for (const string &Fmt : Formats[0])
- formatv(Fmt.c_str(), 1).str();
- for (const string &Fmt : Formats[1])
- formatv(Fmt.c_str(), 1, 2).str();
- for (const string &Fmt : Formats[2])
- formatv(Fmt.c_str(), 1, 2, 3).str();
- for (const string &Fmt : Formats[3])
- formatv(Fmt.c_str(), 1, 2, 3, 4).str();
- for (const string &Fmt : Formats[4])
- formatv(Fmt.c_str(), 1, 2, 3, 4, 5).str();
- }
-}
-
-BENCHMARK(BM_FormatVariadic);
-
-BENCHMARK_MAIN();
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index f31ad70021579e..595f2cf559a428 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -67,20 +67,23 @@ class formatv_object_base {
protected:
StringRef Fmt;
ArrayRef<support::detail::format_adapter *> Adapters;
- bool Validate;
+
+ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+ size_t &Align, char &Pad);
+
+ static std::pair<ReplacementItem, StringRef>
+ splitLiteralAndReplacement(StringRef Fmt);
formatv_object_base(StringRef Fmt,
- ArrayRef<support::detail::format_adapter *> Adapters,
- bool Validate)
- : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
+ ArrayRef<support::detail::format_adapter *> Adapters)
+ : Fmt(Fmt), Adapters(Adapters) {}
formatv_object_base(formatv_object_base const &rhs) = delete;
formatv_object_base(formatv_object_base &&rhs) = default;
public:
void format(raw_ostream &S) const {
- const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
- for (const auto &R : Replacements) {
+ for (auto &R : parseFormatString(Fmt)) {
if (R.Type == ReplacementType::Empty)
continue;
if (R.Type == ReplacementType::Literal) {
@@ -98,10 +101,9 @@ class formatv_object_base {
Align.format(S, R.Options);
}
}
+ static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
- // Parse and optionally validate format string (in debug builds).
- static SmallVector<ReplacementItem, 2>
- parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
+ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
std::string str() const {
std::string Result;
@@ -147,8 +149,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
};
public:
- formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
- : formatv_object_base(Fmt, ParameterPointers, Validate),
+ formatv_object(StringRef Fmt, Tuple &&Params)
+ : formatv_object_base(Fmt, ParameterPointers),
Parameters(std::move(Params)) {
ParameterPointers = std::apply(create_adapters(), Parameters);
}
@@ -245,22 +247,15 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
// assertion. Otherwise, it will try to do something reasonable, but in general
// the details of what that is are undefined.
//
-
-// formatv() with validation enable/disable controlled by the first argument.
template <typename... Ts>
-inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
+inline auto formatv(const char *Fmt, Ts &&...Vals)
-> formatv_object<decltype(std::make_tuple(
support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
using ParamTuple = decltype(std::make_tuple(
support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
- auto Params = std::make_tuple(
- support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
- return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
-}
-
-// formatv() with validation enabled.
-template <typename... Ts> inline auto formatv(const char *Fmt, Ts &&...Vals) {
- return formatv<Ts...>(true, Fmt, std::forward<Ts>(Vals)...);
+ return formatv_object<ParamTuple>(
+ Fmt, std::make_tuple(support::detail::build_format_adapter(
+ std::forward<Ts>(Vals))...));
}
} // end namespace llvm
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index 26d2b549136e43..e25d036cdf1e8c 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
LLVM_BUILTIN_UNREACHABLE;
}
-static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
- size_t &Align, char &Pad) {
+bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+ size_t &Align, char &Pad) {
Where = AlignStyle::Right;
Align = 0;
Pad = ' ';
@@ -35,7 +35,8 @@ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
if (Spec.size() > 1) {
// A maximum of 2 characters at the beginning can be used for something
- // other than the width.
+ // other
+ // than the width.
// If Spec[1] is a loc char, then Spec[0] is a pad char and Spec[2:...]
// contains the width.
// Otherwise, if Spec[0] is a loc char, then Spec[1:...] contains the width.
@@ -54,7 +55,8 @@ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
return !Failed;
}
-static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
+std::optional<ReplacementItem>
+formatv_object_base::parseReplacementItem(StringRef Spec) {
StringRef RepString = Spec.trim("{}");
// If the replacement sequence does not start with a non-negative integer,
@@ -80,14 +82,15 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
RepString = StringRef();
}
RepString = RepString.trim();
- assert(RepString.empty() &&
- "Unexpected characters found in replacement string!");
+ if (!RepString.empty()) {
+ assert(false && "Unexpected characters found in replacement string!");
+ }
return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
}
-static std::pair<ReplacementItem, StringRef>
-splitLiteralAndReplacement(StringRef Fmt) {
+std::pair<ReplacementItem, StringRef>
+formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
while (!Fmt.empty()) {
// Everything up until the first brace is a literal.
if (Fmt.front() != '{') {
@@ -140,77 +143,15 @@ splitLiteralAndReplacement(StringRef Fmt) {
return std::make_pair(ReplacementItem{Fmt}, StringRef());
}
-#ifndef NDEBUG
-#define ENABLE_VALIDATION 1
-#else
-#define ENABLE_VALIDATION 0 // Conveniently enable validation in release mode.
-#endif
-
SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
- bool Validate) {
+formatv_object_base::parseFormatString(StringRef Fmt) {
SmallVector<ReplacementItem, 2> Replacements;
-
-#if ENABLE_VALIDATION
- const StringRef SavedFmtStr = Fmt;
- size_t NumExpectedArgs = 0;
-#endif
-
+ ReplacementItem I;
while (!Fmt.empty()) {
- ReplacementItem I;
std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
if (I.Type != ReplacementType::Empty)
Replacements.push_back(I);
-#if ENABLE_VALIDATION
- if (I.Type == ReplacementType::Format)
- NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
-#endif
- }
-
-#if ENABLE_VALIDATION
- if (!Validate)
- return Replacements;
-
- // Perform additional validation. Verify that the number of arguments matches
- // the number of replacement indices and that there are no holes in the
- // replacement indices.
-
- // When validation fails, return an array of replacement items that
- // will print an error message as the outout of this formatv() (used when
- // validation is enabled in release mode).
- auto getErrorReplacements = [SavedFmtStr](StringLiteral ErrorMsg) {
- return SmallVector<ReplacementItem, 2>{
- ReplacementItem("Invalid formatv() call: "), ReplacementItem(ErrorMsg),
- ReplacementItem(" for format string: "), ReplacementItem(SavedFmtStr)};
- };
-
- if (NumExpectedArgs != NumArgs) {
- errs() << formatv(
- "Expected {0} Args, but got {1} for format string '{2}'\n",
- NumExpectedArgs, NumArgs, SavedFmtStr);
- assert(0 && "Invalid formatv() call");
- return getErrorReplacements("Unexpected number of arguments");
- }
-
- // Find the number of unique indices seen. All replacement indices
- // are < NumExpectedArgs.
- SmallVector<bool> Indices(NumExpectedArgs);
- size_t Count = 0;
- for (const ReplacementItem &I : Replacements) {
- if (I.Type != ReplacementType::Format || Indices[I.Index])
- continue;
- Indices[I.Index] = true;
- ++Count;
- }
-
- if (Count != NumExpectedArgs) {
- errs() << formatv(
- "Replacement field indices cannot have holes for format string '{0}'\n",
- SavedFmtStr);
- assert(0 && "Invalid format string");
- return getErrorReplacements("Replacement indices have holes");
}
-#endif // ENABLE_VALIDATION
return Replacements;
}
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 6ee0d924867419..a78b25c53d7e43 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -9,11 +9,9 @@
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FormatAdapters.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
using namespace llvm;
-using ::testing::HasSubstr;
// Compile-time tests templates in the detail namespace.
namespace {
@@ -37,19 +35,14 @@ struct NoFormat {};
static_assert(uses_missing_provider<NoFormat>::value, "");
}
-// Helper to parse format string with no validation.
-static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
- return formatv_object_base::parseFormatString(Fmt, 0, false);
-}
-
TEST(FormatVariadicTest, EmptyFormatString) {
- auto Replacements = parseFormatString("");
+ auto Replacements = formatv_object_base::parseFormatString("");
EXPECT_EQ(0U, Replacements.size());
}
TEST(FormatVariadicTest, NoReplacements) {
const StringRef kFormatString = "This is a test";
- auto Replacements = parseFormatString(kFormatString);
+ auto Replacements = formatv_object_base::parseFormatString(kFormatString);
ASSERT_EQ(1U, Replacements.size());
EXPECT_EQ(kFormatString, Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -57,25 +50,25 @@ TEST(FormatVariadicTest, NoReplacements) {
TEST(FormatVariadicTest, EscapedBrace) {
// {{ should be replaced with {
- auto Replacements = parseFormatString("{{");
+ auto Replacements = formatv_object_base::parseFormatString("{{");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("{", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// An even number N of braces should be replaced with N/2 braces.
- Replacements = parseFormatString("{{{{{{");
+ Replacements = formatv_object_base::parseFormatString("{{{{{{");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("{{{", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// } does not require doubling up.
- Replacements = parseFormatString("}");
+ Replacements = formatv_object_base::parseFormatString("}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("}", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// } does not require doubling up.
- Replacements = parseFormatString("}}}");
+ Replacements = formatv_object_base::parseFormatString("}}}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("}}}", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -83,14 +76,14 @@ TEST(FormatVariadicTest, EscapedBrace) {
TEST(FormatVariadicTest, ValidReplacementSequence) {
// 1. Simple replacement - parameter index only
- auto Replacements = parseFormatString("{0}");
+ auto Replacements = formatv_object_base::parseFormatString("{0}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
EXPECT_EQ(0u, Replacements[0].Align);
EXPECT_EQ("", Replacements[0].Options);
- Replacements = parseFormatString("{1}");
+ Replacements = formatv_object_base::parseFormatString("{1}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(1u, Replacements[0].Index);
@@ -99,7 +92,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 2. Parameter index with right alignment
- Replacements = parseFormatString("{0,3}");
+ Replacements = formatv_object_base::parseFormatString("{0,3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -108,7 +101,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 3. And left alignment
- Replacements = parseFormatString("{0,-3}");
+ Replacements = formatv_object_base::parseFormatString("{0,-3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -117,7 +110,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 4. And center alignment
- Replacements = parseFormatString("{0,=3}");
+ Replacements = formatv_object_base::parseFormatString("{0,=3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -126,7 +119,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 4. Parameter index with option string
- Replacements = parseFormatString("{0:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -135,7 +128,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// 5. Parameter index with alignment before option string
- Replacements = parseFormatString("{0,-3:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,-3:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -144,7 +137,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// 7. Parameter indices, options, and alignment can all have whitespace.
- Replacements = parseFormatString("{ 0, -3 : foo }");
+ Replacements = formatv_object_base::parseFormatString("{ 0, -3 : foo }");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -154,7 +147,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
// 8. Everything after the first option specifier is part of the style, even
// if it contains another option specifier.
- Replacements = parseFormatString("{0:0:1}");
+ Replacements = formatv_object_base::parseFormatString("{0:0:1}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0:0:1", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -164,7 +157,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("0:1", Replacements[0].Options);
// 9. Custom padding character
- Replacements = parseFormatString("{0,p+4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,p+4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,p+4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -175,7 +168,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// Format string special characters are allowed as padding character
- Replacements = parseFormatString("{0,-+4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,-+4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,-+4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -185,7 +178,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ('-', Replacements[0].Pad);
EXPECT_EQ("foo", Replacements[0].Options);
- Replacements = parseFormatString("{0,+-4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,+-4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,+-4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -195,7 +188,7 @@ TEST(FormatVariadicTest, ValidReplacement...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Mehdi Amini (joker-eph) ChangesReverts llvm/llvm-project#105745 Some bots are broken apparently. Patch is 23.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106589.diff 7 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 4f30b2a0e7e7da..8f4bd17afc8581 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -1401,10 +1401,7 @@ void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
ErrnoNote =
llvm::formatv("After calling '{0}' {1}", FunctionName, ErrnoNote);
} else {
- // Disable formatv() validation as the case note may not always have the
- // {0} placeholder for function name.
- CaseNote =
- llvm::formatv(false, Case.getNote().str().c_str(), FunctionName);
+ CaseNote = llvm::formatv(Case.getNote().str().c_str(), FunctionName);
}
const SVal RV = Call.getReturnValue();
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index e3366e6f3ffe19..713d4ccd3c5975 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -5,4 +5,3 @@ set(LLVM_LINK_COMPONENTS
add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
-add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/FormatVariadicBM.cpp b/llvm/benchmarks/FormatVariadicBM.cpp
deleted file mode 100644
index c03ead400d0d5c..00000000000000
--- a/llvm/benchmarks/FormatVariadicBM.cpp
+++ /dev/null
@@ -1,63 +0,0 @@
-//===- FormatVariadicBM.cpp - formatv() benchmark ---------- --------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "benchmark/benchmark.h"
-#include "llvm/Support/FormatVariadic.h"
-#include <algorithm>
-#include <string>
-#include <vector>
-
-using namespace llvm;
-using namespace std;
-
-// Generate a list of format strings that have `NumReplacements` replacements
-// by permuting the replacements and some literal text.
-static vector<string> getFormatStrings(int NumReplacements) {
- vector<string> Components;
- for (int I = 0; I < NumReplacements; I++)
- Components.push_back("{" + to_string(I) + "}");
- // Intersperse these with some other literal text (_).
- const string_view Literal = "____";
- for (char C : Literal)
- Components.push_back(string(1, C));
-
- vector<string> Formats;
- do {
- string Concat;
- for (const string &C : Components)
- Concat += C;
- Formats.emplace_back(Concat);
- } while (next_permutation(Components.begin(), Components.end()));
- return Formats;
-}
-
-// Generate the set of formats to exercise outside the benchmark code.
-static const vector<vector<string>> Formats = {
- getFormatStrings(1), getFormatStrings(2), getFormatStrings(3),
- getFormatStrings(4), getFormatStrings(5),
-};
-
-// Benchmark formatv() for a variety of format strings and 1-5 replacements.
-static void BM_FormatVariadic(benchmark::State &state) {
- for (auto _ : state) {
- for (const string &Fmt : Formats[0])
- formatv(Fmt.c_str(), 1).str();
- for (const string &Fmt : Formats[1])
- formatv(Fmt.c_str(), 1, 2).str();
- for (const string &Fmt : Formats[2])
- formatv(Fmt.c_str(), 1, 2, 3).str();
- for (const string &Fmt : Formats[3])
- formatv(Fmt.c_str(), 1, 2, 3, 4).str();
- for (const string &Fmt : Formats[4])
- formatv(Fmt.c_str(), 1, 2, 3, 4, 5).str();
- }
-}
-
-BENCHMARK(BM_FormatVariadic);
-
-BENCHMARK_MAIN();
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index f31ad70021579e..595f2cf559a428 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -67,20 +67,23 @@ class formatv_object_base {
protected:
StringRef Fmt;
ArrayRef<support::detail::format_adapter *> Adapters;
- bool Validate;
+
+ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+ size_t &Align, char &Pad);
+
+ static std::pair<ReplacementItem, StringRef>
+ splitLiteralAndReplacement(StringRef Fmt);
formatv_object_base(StringRef Fmt,
- ArrayRef<support::detail::format_adapter *> Adapters,
- bool Validate)
- : Fmt(Fmt), Adapters(Adapters), Validate(Validate) {}
+ ArrayRef<support::detail::format_adapter *> Adapters)
+ : Fmt(Fmt), Adapters(Adapters) {}
formatv_object_base(formatv_object_base const &rhs) = delete;
formatv_object_base(formatv_object_base &&rhs) = default;
public:
void format(raw_ostream &S) const {
- const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
- for (const auto &R : Replacements) {
+ for (auto &R : parseFormatString(Fmt)) {
if (R.Type == ReplacementType::Empty)
continue;
if (R.Type == ReplacementType::Literal) {
@@ -98,10 +101,9 @@ class formatv_object_base {
Align.format(S, R.Options);
}
}
+ static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt);
- // Parse and optionally validate format string (in debug builds).
- static SmallVector<ReplacementItem, 2>
- parseFormatString(StringRef Fmt, size_t NumArgs, bool Validate);
+ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec);
std::string str() const {
std::string Result;
@@ -147,8 +149,8 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
};
public:
- formatv_object(StringRef Fmt, Tuple &&Params, bool Validate)
- : formatv_object_base(Fmt, ParameterPointers, Validate),
+ formatv_object(StringRef Fmt, Tuple &&Params)
+ : formatv_object_base(Fmt, ParameterPointers),
Parameters(std::move(Params)) {
ParameterPointers = std::apply(create_adapters(), Parameters);
}
@@ -245,22 +247,15 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
// assertion. Otherwise, it will try to do something reasonable, but in general
// the details of what that is are undefined.
//
-
-// formatv() with validation enable/disable controlled by the first argument.
template <typename... Ts>
-inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
+inline auto formatv(const char *Fmt, Ts &&...Vals)
-> formatv_object<decltype(std::make_tuple(
support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
using ParamTuple = decltype(std::make_tuple(
support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
- auto Params = std::make_tuple(
- support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
- return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
-}
-
-// formatv() with validation enabled.
-template <typename... Ts> inline auto formatv(const char *Fmt, Ts &&...Vals) {
- return formatv<Ts...>(true, Fmt, std::forward<Ts>(Vals)...);
+ return formatv_object<ParamTuple>(
+ Fmt, std::make_tuple(support::detail::build_format_adapter(
+ std::forward<Ts>(Vals))...));
}
} // end namespace llvm
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index 26d2b549136e43..e25d036cdf1e8c 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -25,8 +25,8 @@ static std::optional<AlignStyle> translateLocChar(char C) {
LLVM_BUILTIN_UNREACHABLE;
}
-static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
- size_t &Align, char &Pad) {
+bool formatv_object_base::consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
+ size_t &Align, char &Pad) {
Where = AlignStyle::Right;
Align = 0;
Pad = ' ';
@@ -35,7 +35,8 @@ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
if (Spec.size() > 1) {
// A maximum of 2 characters at the beginning can be used for something
- // other than the width.
+ // other
+ // than the width.
// If Spec[1] is a loc char, then Spec[0] is a pad char and Spec[2:...]
// contains the width.
// Otherwise, if Spec[0] is a loc char, then Spec[1:...] contains the width.
@@ -54,7 +55,8 @@ static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
return !Failed;
}
-static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
+std::optional<ReplacementItem>
+formatv_object_base::parseReplacementItem(StringRef Spec) {
StringRef RepString = Spec.trim("{}");
// If the replacement sequence does not start with a non-negative integer,
@@ -80,14 +82,15 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
RepString = StringRef();
}
RepString = RepString.trim();
- assert(RepString.empty() &&
- "Unexpected characters found in replacement string!");
+ if (!RepString.empty()) {
+ assert(false && "Unexpected characters found in replacement string!");
+ }
return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
}
-static std::pair<ReplacementItem, StringRef>
-splitLiteralAndReplacement(StringRef Fmt) {
+std::pair<ReplacementItem, StringRef>
+formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
while (!Fmt.empty()) {
// Everything up until the first brace is a literal.
if (Fmt.front() != '{') {
@@ -140,77 +143,15 @@ splitLiteralAndReplacement(StringRef Fmt) {
return std::make_pair(ReplacementItem{Fmt}, StringRef());
}
-#ifndef NDEBUG
-#define ENABLE_VALIDATION 1
-#else
-#define ENABLE_VALIDATION 0 // Conveniently enable validation in release mode.
-#endif
-
SmallVector<ReplacementItem, 2>
-formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
- bool Validate) {
+formatv_object_base::parseFormatString(StringRef Fmt) {
SmallVector<ReplacementItem, 2> Replacements;
-
-#if ENABLE_VALIDATION
- const StringRef SavedFmtStr = Fmt;
- size_t NumExpectedArgs = 0;
-#endif
-
+ ReplacementItem I;
while (!Fmt.empty()) {
- ReplacementItem I;
std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
if (I.Type != ReplacementType::Empty)
Replacements.push_back(I);
-#if ENABLE_VALIDATION
- if (I.Type == ReplacementType::Format)
- NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
-#endif
- }
-
-#if ENABLE_VALIDATION
- if (!Validate)
- return Replacements;
-
- // Perform additional validation. Verify that the number of arguments matches
- // the number of replacement indices and that there are no holes in the
- // replacement indices.
-
- // When validation fails, return an array of replacement items that
- // will print an error message as the outout of this formatv() (used when
- // validation is enabled in release mode).
- auto getErrorReplacements = [SavedFmtStr](StringLiteral ErrorMsg) {
- return SmallVector<ReplacementItem, 2>{
- ReplacementItem("Invalid formatv() call: "), ReplacementItem(ErrorMsg),
- ReplacementItem(" for format string: "), ReplacementItem(SavedFmtStr)};
- };
-
- if (NumExpectedArgs != NumArgs) {
- errs() << formatv(
- "Expected {0} Args, but got {1} for format string '{2}'\n",
- NumExpectedArgs, NumArgs, SavedFmtStr);
- assert(0 && "Invalid formatv() call");
- return getErrorReplacements("Unexpected number of arguments");
- }
-
- // Find the number of unique indices seen. All replacement indices
- // are < NumExpectedArgs.
- SmallVector<bool> Indices(NumExpectedArgs);
- size_t Count = 0;
- for (const ReplacementItem &I : Replacements) {
- if (I.Type != ReplacementType::Format || Indices[I.Index])
- continue;
- Indices[I.Index] = true;
- ++Count;
- }
-
- if (Count != NumExpectedArgs) {
- errs() << formatv(
- "Replacement field indices cannot have holes for format string '{0}'\n",
- SavedFmtStr);
- assert(0 && "Invalid format string");
- return getErrorReplacements("Replacement indices have holes");
}
-#endif // ENABLE_VALIDATION
return Replacements;
}
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 6ee0d924867419..a78b25c53d7e43 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -9,11 +9,9 @@
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FormatAdapters.h"
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
using namespace llvm;
-using ::testing::HasSubstr;
// Compile-time tests templates in the detail namespace.
namespace {
@@ -37,19 +35,14 @@ struct NoFormat {};
static_assert(uses_missing_provider<NoFormat>::value, "");
}
-// Helper to parse format string with no validation.
-static SmallVector<ReplacementItem, 2> parseFormatString(StringRef Fmt) {
- return formatv_object_base::parseFormatString(Fmt, 0, false);
-}
-
TEST(FormatVariadicTest, EmptyFormatString) {
- auto Replacements = parseFormatString("");
+ auto Replacements = formatv_object_base::parseFormatString("");
EXPECT_EQ(0U, Replacements.size());
}
TEST(FormatVariadicTest, NoReplacements) {
const StringRef kFormatString = "This is a test";
- auto Replacements = parseFormatString(kFormatString);
+ auto Replacements = formatv_object_base::parseFormatString(kFormatString);
ASSERT_EQ(1U, Replacements.size());
EXPECT_EQ(kFormatString, Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -57,25 +50,25 @@ TEST(FormatVariadicTest, NoReplacements) {
TEST(FormatVariadicTest, EscapedBrace) {
// {{ should be replaced with {
- auto Replacements = parseFormatString("{{");
+ auto Replacements = formatv_object_base::parseFormatString("{{");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("{", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// An even number N of braces should be replaced with N/2 braces.
- Replacements = parseFormatString("{{{{{{");
+ Replacements = formatv_object_base::parseFormatString("{{{{{{");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("{{{", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// } does not require doubling up.
- Replacements = parseFormatString("}");
+ Replacements = formatv_object_base::parseFormatString("}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("}", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
// } does not require doubling up.
- Replacements = parseFormatString("}}}");
+ Replacements = formatv_object_base::parseFormatString("}}}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("}}}", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
@@ -83,14 +76,14 @@ TEST(FormatVariadicTest, EscapedBrace) {
TEST(FormatVariadicTest, ValidReplacementSequence) {
// 1. Simple replacement - parameter index only
- auto Replacements = parseFormatString("{0}");
+ auto Replacements = formatv_object_base::parseFormatString("{0}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
EXPECT_EQ(0u, Replacements[0].Align);
EXPECT_EQ("", Replacements[0].Options);
- Replacements = parseFormatString("{1}");
+ Replacements = formatv_object_base::parseFormatString("{1}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(1u, Replacements[0].Index);
@@ -99,7 +92,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 2. Parameter index with right alignment
- Replacements = parseFormatString("{0,3}");
+ Replacements = formatv_object_base::parseFormatString("{0,3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -108,7 +101,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 3. And left alignment
- Replacements = parseFormatString("{0,-3}");
+ Replacements = formatv_object_base::parseFormatString("{0,-3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -117,7 +110,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 4. And center alignment
- Replacements = parseFormatString("{0,=3}");
+ Replacements = formatv_object_base::parseFormatString("{0,=3}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -126,7 +119,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("", Replacements[0].Options);
// 4. Parameter index with option string
- Replacements = parseFormatString("{0:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -135,7 +128,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// 5. Parameter index with alignment before option string
- Replacements = parseFormatString("{0,-3:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,-3:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -144,7 +137,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// 7. Parameter indices, options, and alignment can all have whitespace.
- Replacements = parseFormatString("{ 0, -3 : foo }");
+ Replacements = formatv_object_base::parseFormatString("{ 0, -3 : foo }");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
EXPECT_EQ(0u, Replacements[0].Index);
@@ -154,7 +147,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
// 8. Everything after the first option specifier is part of the style, even
// if it contains another option specifier.
- Replacements = parseFormatString("{0:0:1}");
+ Replacements = formatv_object_base::parseFormatString("{0:0:1}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0:0:1", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -164,7 +157,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("0:1", Replacements[0].Options);
// 9. Custom padding character
- Replacements = parseFormatString("{0,p+4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,p+4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,p+4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -175,7 +168,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ("foo", Replacements[0].Options);
// Format string special characters are allowed as padding character
- Replacements = parseFormatString("{0,-+4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,-+4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,-+4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -185,7 +178,7 @@ TEST(FormatVariadicTest, ValidReplacementSequence) {
EXPECT_EQ('-', Replacements[0].Pad);
EXPECT_EQ("foo", Replacements[0].Options);
- Replacements = parseFormatString("{0,+-4:foo}");
+ Replacements = formatv_object_base::parseFormatString("{0,+-4:foo}");
ASSERT_EQ(1u, Replacements.size());
EXPECT_EQ("0,+-4:foo", Replacements[0].Spec);
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -195,7 +188,7 @@ TEST(FormatVariadicTest, ValidReplacement...
[truncated]
|
|
The fix for the bot failures has already been committed (with #106570), so this revert is not needed. @joker-eph I'll revert the revert and lets the bots catch up. |
|
Looks good! Sorry I missed the forward fix you landed in the meantime. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/5057 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #105745
Some bots are broken apparently.