[llvm] 2f7ffba - [Support] Fix bugs in formatv automatic index assignment (#108384)

[via llvm-commits]

Author: Rahul Joshi
Date: 2024-09-16T21:48:05-07:00
New Revision: 2f7ffbaad3e7bd14b7a82d4887ef1640272ba1be

URL: https://github.com/llvm/llvm-project/commit/2f7ffbaad3e7bd14b7a82d4887ef1640272ba1be
DIFF: https://github.com/llvm/llvm-project/commit/2f7ffbaad3e7bd14b7a82d4887ef1640272ba1be.diff

LOG: [Support] Fix bugs in formatv automatic index assignment (#108384)

Fix bugs found when actually trying to use formatv() automatic index
assignment in IntrinsicEmitter.cpp:
- Assign automatic index only for `ReplacementType::Format`.
- Make the check for all replacement indices being either automatic orexplicit more accurate.
  The existing check fails for formatv("{}{0}{}", 0, 1) (added as a unit test). Explicitly track if we
  have seen any explicit and any automatic index instead.

Added: 
    

Modified: 
    llvm/lib/Support/FormatVariadic.cpp
    llvm/unittests/Support/FormatVariadicTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
index 3240b1c0f4e413..7eb10887940513 100644
--- a/llvm/lib/Support/FormatVariadic.cpp
+++ b/llvm/lib/Support/FormatVariadic.cpp
@@ -148,6 +148,7 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
 #if ENABLE_VALIDATION
   const StringRef SavedFmtStr = Fmt;
   unsigned NumExpectedArgs = 0;
+  bool HasExplicitIndex = false;
 #endif
 
   while (!Fmt.empty()) {
@@ -155,14 +156,17 @@ 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 (I->Type == ReplacementType::Format) {
+      if (I->Index == ~0U)
+        I->Index = NextAutomaticIndex++;
 #if ENABLE_VALIDATION
-    if (I->Type == ReplacementType::Format)
+      else
+        HasExplicitIndex = true;
       NumExpectedArgs = std::max(NumExpectedArgs, I->Index + 1);
 #endif
+    }
+
+    Replacements.emplace_back(*I);
   }
 
 #if ENABLE_VALIDATION
@@ -208,9 +212,8 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
     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) {
+  // Fail validation if we see both automatic index and explicit index.
+  if (NextAutomaticIndex != 0 && HasExplicitIndex) {
     errs() << formatv(
         "Cannot mix automatic and explicit indices for format string '{}'\n",
         SavedFmtStr);

diff  --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
index 8e326e1f2b4396..e745f99a5a6c4e 100644
--- a/llvm/unittests/Support/FormatVariadicTest.cpp
+++ b/llvm/unittests/Support/FormatVariadicTest.cpp
@@ -283,6 +283,15 @@ TEST(FormatVariadicTest, AutomaticIndices) {
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
   EXPECT_EQ(0u, Replacements[0].Index);
 
+  Replacements = parseFormatString("{}bar{}");
+  ASSERT_EQ(3u, Replacements.size());
+  EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
+  EXPECT_EQ(0u, Replacements[0].Index);
+  EXPECT_EQ(ReplacementType::Literal, Replacements[1].Type);
+  EXPECT_EQ("bar", Replacements[1].Spec);
+  EXPECT_EQ(ReplacementType::Format, Replacements[2].Type);
+  EXPECT_EQ(1u, Replacements[2].Index);
+
   Replacements = parseFormatString("{}{}");
   ASSERT_EQ(2u, Replacements.size());
   EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -760,6 +769,8 @@ TEST(FormatVariadicTest, Validate) {
                "Replacement field indices cannot have holes");
   EXPECT_DEATH(formatv("{}{1}", 0, 1).str(),
                "Cannot mix automatic and explicit indices");
+  EXPECT_DEATH(formatv("{}{0}{}", 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
@@ -768,6 +779,7 @@ TEST(FormatVariadicTest, Validate) {
   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");
+  EXPECT_EQ(formatv("{}{0}{}", 0, 1).str(), "001");
 #endif // NDEBUG
 }