[Support] Add automatic index assignment in formatv

[jurahul]

Make index in replacement field optional. It will be automatically assigned in incremental order by formatv.

Make mixed use of automatic and explicit indices an error that will fail validation.
Adopt uses of formatv() within FormatVariadic to use automatic index.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Rahul Joshi (jurahul)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/107459.diff

3 Files Affected:

• (modified) llvm/include/llvm/Support/FormatVariadic.h (+6-3)
• (modified) llvm/lib/Support/FormatVariadic.cpp (+29-12)
• (modified) llvm/unittests/Support/FormatVariadicTest.cpp (+46-1)

diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
index 005d26f02d8fd9..13c47cae3ecb5c 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,11 @@ 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 undefined.
 // 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
 }
 

[River707]

This seems really useful, and also helps prevent all of the issues with numbered indices going out of date (or unused).