[llvm] [Support] Add automatic index assignment in formatv (PR #107459)
Rahul Joshi via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 11 15:39:20 PDT 2024
https://github.com/jurahul updated https://github.com/llvm/llvm-project/pull/107459
>From 7e731c61760524a39486270b8831d74e9c5f1491 Mon Sep 17 00:00:00 2001
From: Rahul Joshi <rjoshi at nvidia.com>
Date: Thu, 5 Sep 2024 12:01:42 -0700
Subject: [PATCH] [Support] Add automatic index assignment in formatv
Make index in replacement field optional. It will be automatically
assigned in incremental order by formatv.
Make mixed use of automatic and explicit indexes an error that will
fail validation.
Adopt uses of formatv() within FormatVariadic to use automatic indexes.
---
llvm/include/llvm/Support/FormatVariadic.h | 10 ++--
llvm/lib/Support/FormatVariadic.cpp | 41 +++++++++++-----
llvm/unittests/Support/FormatVariadicTest.cpp | 47 ++++++++++++++++++-
3 files changed, 82 insertions(+), 16 deletions(-)
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 005d26f02d8fd9..d0e647e1403bdf 100644
--- a/llvm/include/llvm/Support/FormatVariadic.h
+++ b/llvm/include/llvm/Support/FormatVariadic.h
@@ -167,7 +167,7 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
// Formats textual output. `Fmt` is a string consisting of one or more
// replacement sequences with the following grammar:
//
-// rep_field ::= "{" index ["," layout] [":" format] "}"
+// rep_field ::= "{" [index] ["," layout] [":" format] "}"
// index ::= <non-negative integer>
// layout ::= [[[char]loc]width]
// format ::= <any string not containing "{" or "}">
@@ -175,8 +175,12 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
// loc ::= "-" | "=" | "+"
// width ::= <positive integer>
//
-// index - A non-negative integer specifying the index of the item in the
-// parameter pack to print. Any other value is invalid.
+// index - An optional non-negative integer specifying the index of the item
+// in the parameter pack to print. Any other value is invalid. If its
+// not specified, it will be automatically assigned a value based on
+// the order of rep_field seen in the format string. Note that mixing
+// automatic and explicit index in the same call is an error and will
+// fail validation in assert-enabled builds.
// layout - A string controlling how the field is laid out within the available
// space.
// format - A type-dependent string used to provide additional options to
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index 9056466190284b..3240b1c0f4e413 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -63,16 +63,18 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
unsigned Align = 0;
AlignStyle Where = AlignStyle::Right;
StringRef Options;
- unsigned Index = 0;
+ unsigned Index = ~0U;
RepString = RepString.trim();
- if (RepString.consumeInteger(0, Index)) {
- assert(false && "Invalid replacement sequence index!");
- return std::nullopt;
- }
+
+ // If index is not specified, keep it ~0U to indicate unresolved index.
+ RepString.consumeInteger(0, Index);
RepString = RepString.trim();
+
if (RepString.consume_front(",")) {
- if (!consumeFieldLayout(RepString, Where, Align, Pad))
+ if (!consumeFieldLayout(RepString, Where, Align, Pad)) {
assert(false && "Invalid replacement field layout specification!");
+ return std::nullopt;
+ }
}
RepString = RepString.trim();
if (RepString.consume_front(":")) {
@@ -80,8 +82,10 @@ 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(0 && "Unexpected characters found in replacement string!");
+ return std::nullopt;
+ }
return ReplacementItem(Spec, Index, Align, Where, Pad, Options);
}
@@ -139,6 +143,7 @@ SmallVector<ReplacementItem, 2>
formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
bool Validate) {
SmallVector<ReplacementItem, 2> Replacements;
+ unsigned NextAutomaticIndex = 0;
#if ENABLE_VALIDATION
const StringRef SavedFmtStr = Fmt;
@@ -150,6 +155,9 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
if (!I)
continue;
+ if (I->Index == ~0U)
+ I->Index = NextAutomaticIndex++;
+
Replacements.emplace_back(*I);
#if ENABLE_VALIDATION
if (I->Type == ReplacementType::Format)
@@ -175,9 +183,8 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
};
if (NumExpectedArgs != NumArgs) {
- errs() << formatv(
- "Expected {0} Args, but got {1} for format string '{2}'\n",
- NumExpectedArgs, NumArgs, SavedFmtStr);
+ errs() << formatv("Expected {} Args, but got {} for format string '{}'\n",
+ NumExpectedArgs, NumArgs, SavedFmtStr);
assert(0 && "Invalid formatv() call");
return getErrorReplacements("Unexpected number of arguments");
}
@@ -195,11 +202,21 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
if (Count != NumExpectedArgs) {
errs() << formatv(
- "Replacement field indices cannot have holes for format string '{0}'\n",
+ "Replacement field indices cannot have holes for format string '{}'\n",
SavedFmtStr);
assert(0 && "Invalid format string");
return getErrorReplacements("Replacement indices have holes");
}
+
+ // If we had automatic numbering of replacement indices, verify that all
+ // indices used automatic numbering.
+ if (NextAutomaticIndex != 0 && NextAutomaticIndex != Count) {
+ errs() << formatv(
+ "Cannot mix automatic and explicit indices for format string '{}'\n",
+ SavedFmtStr);
+ assert(0 && "Invalid format string");
+ return getErrorReplacements("Cannot mix automatic and explicit indices");
+ }
#endif // ENABLE_VALIDATION
return Replacements;
}
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 4f3d1791c0018d..68938480ecfe20 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -269,7 +269,7 @@ TEST(FormatVariadicTest, MultipleReplacements) {
EXPECT_EQ(ReplacementType::Literal, Replacements[3].Type);
EXPECT_EQ("-", Replacements[3].Spec);
- // {2:bar,-3} - Options=bar, Align=-3
+ // {2,-3:bar} - Options=bar, Align=-3
EXPECT_EQ(ReplacementType::Format, Replacements[4].Type);
EXPECT_EQ(2u, Replacements[4].Index);
EXPECT_EQ(3u, Replacements[4].Width);
@@ -277,6 +277,42 @@ TEST(FormatVariadicTest, MultipleReplacements) {
EXPECT_EQ("bar", Replacements[4].Options);
}
+TEST(FormatVariadicTest, AutomaticIndices) {
+ auto Replacements = parseFormatString("{}");
+ ASSERT_EQ(1u, Replacements.size());
+ EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
+ EXPECT_EQ(0u, Replacements[0].Index);
+
+ Replacements = parseFormatString("{}{}");
+ ASSERT_EQ(2u, Replacements.size());
+ EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
+ EXPECT_EQ(0u, Replacements[0].Index);
+ EXPECT_EQ(ReplacementType::Format, Replacements[1].Type);
+ EXPECT_EQ(1u, Replacements[1].Index);
+
+ Replacements = parseFormatString("{}{:foo}{,-3:bar}");
+ ASSERT_EQ(3u, Replacements.size());
+ EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
+ EXPECT_EQ(0u, Replacements[0].Index);
+ EXPECT_EQ(0u, Replacements[0].Width);
+ EXPECT_EQ(AlignStyle::Right, Replacements[0].Where);
+ EXPECT_EQ("", Replacements[0].Options);
+
+ // {:foo} - Options=foo
+ EXPECT_EQ(ReplacementType::Format, Replacements[1].Type);
+ EXPECT_EQ(1u, Replacements[1].Index);
+ EXPECT_EQ(0u, Replacements[1].Width);
+ EXPECT_EQ(AlignStyle::Right, Replacements[1].Where);
+ EXPECT_EQ("foo", Replacements[1].Options);
+
+ // {,-3:bar} - Options=bar, Align=-3
+ EXPECT_EQ(ReplacementType::Format, Replacements[2].Type);
+ EXPECT_EQ(2u, Replacements[2].Index);
+ EXPECT_EQ(3u, Replacements[2].Width);
+ EXPECT_EQ(AlignStyle::Left, Replacements[2].Where);
+ EXPECT_EQ("bar", Replacements[2].Options);
+}
+
TEST(FormatVariadicTest, FormatNoReplacements) {
EXPECT_EQ("", formatv("").str());
EXPECT_EQ("Test", formatv("Test").str());
@@ -291,6 +327,12 @@ TEST(FormatVariadicTest, FormatBasicTypesOneReplacement) {
EXPECT_EQ("Test3", formatv("{0}", std::string("Test3")).str());
}
+TEST(FormatVariadicTest, FormatAutomaticIndices) {
+ EXPECT_EQ("1", formatv("{}", 1).str());
+ EXPECT_EQ("c1", formatv("{}{}", 'c', 1).str());
+ EXPECT_EQ("c-1rrr-0xFF", formatv("{}-{,r-4}-{:X}", 'c', 1, 255).str());
+}
+
TEST(FormatVariadicTest, IntegralHexFormatting) {
// 1. Trivial cases. Make sure hex is not the default.
EXPECT_EQ("0", formatv("{0}", 0).str());
@@ -717,6 +759,8 @@ TEST(FormatVariadicTest, Validate) {
EXPECT_DEATH(formatv("{0}", 1, 2).str(), "Expected 1 Args, but got 2");
EXPECT_DEATH(formatv("{0} {2}", 1, 2, 3).str(),
"Replacement field indices cannot have holes");
+ EXPECT_DEATH(formatv("{}{1}", 0, 1).str(),
+ "Cannot mix automatic and explicit indices");
#else // GTEST_HAS_DEATH_TEST
GTEST_SKIP() << "No support for EXPECT_DEATH";
#endif // GTEST_HAS_DEATH_TEST
@@ -724,6 +768,7 @@ TEST(FormatVariadicTest, Validate) {
// If asserts are disabled, verify that validation is disabled.
EXPECT_EQ(formatv("{0}", 1, 2).str(), "1");
EXPECT_EQ(formatv("{0} {2}", 1, 2, 3).str(), "1 3");
+ EXPECT_EQ(formatv("{}{1}", 0, 1).str(), "01");
#endif // NDEBUG
}
More information about the llvm-commits
mailing list