Skip to content

Commit 2f7ffba

Browse files
authored
[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.
1 parent 95a0b4f commit 2f7ffba

File tree

2 files changed

+23
-8
lines changed

2 files changed

+23
-8
lines changed

llvm/lib/Support/FormatVariadic.cpp

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -148,21 +148,25 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
148148
#if ENABLE_VALIDATION
149149
const StringRef SavedFmtStr = Fmt;
150150
unsigned NumExpectedArgs = 0;
151+
bool HasExplicitIndex = false;
151152
#endif
152153

153154
while (!Fmt.empty()) {
154155
std::optional<ReplacementItem> I;
155156
std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
156157
if (!I)
157158
continue;
158-
if (I->Index == ~0U)
159-
I->Index = NextAutomaticIndex++;
160-
161-
Replacements.emplace_back(*I);
159+
if (I->Type == ReplacementType::Format) {
160+
if (I->Index == ~0U)
161+
I->Index = NextAutomaticIndex++;
162162
#if ENABLE_VALIDATION
163-
if (I->Type == ReplacementType::Format)
163+
else
164+
HasExplicitIndex = true;
164165
NumExpectedArgs = std::max(NumExpectedArgs, I->Index + 1);
165166
#endif
167+
}
168+
169+
Replacements.emplace_back(*I);
166170
}
167171

168172
#if ENABLE_VALIDATION
@@ -208,9 +212,8 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
208212
return getErrorReplacements("Replacement indices have holes");
209213
}
210214

211-
// If we had automatic numbering of replacement indices, verify that all
212-
// indices used automatic numbering.
213-
if (NextAutomaticIndex != 0 && NextAutomaticIndex != Count) {
215+
// Fail validation if we see both automatic index and explicit index.
216+
if (NextAutomaticIndex != 0 && HasExplicitIndex) {
214217
errs() << formatv(
215218
"Cannot mix automatic and explicit indices for format string '{}'\n",
216219
SavedFmtStr);

llvm/unittests/Support/FormatVariadicTest.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,15 @@ TEST(FormatVariadicTest, AutomaticIndices) {
283283
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
284284
EXPECT_EQ(0u, Replacements[0].Index);
285285

286+
Replacements = parseFormatString("{}bar{}");
287+
ASSERT_EQ(3u, Replacements.size());
288+
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
289+
EXPECT_EQ(0u, Replacements[0].Index);
290+
EXPECT_EQ(ReplacementType::Literal, Replacements[1].Type);
291+
EXPECT_EQ("bar", Replacements[1].Spec);
292+
EXPECT_EQ(ReplacementType::Format, Replacements[2].Type);
293+
EXPECT_EQ(1u, Replacements[2].Index);
294+
286295
Replacements = parseFormatString("{}{}");
287296
ASSERT_EQ(2u, Replacements.size());
288297
EXPECT_EQ(ReplacementType::Format, Replacements[0].Type);
@@ -760,6 +769,8 @@ TEST(FormatVariadicTest, Validate) {
760769
"Replacement field indices cannot have holes");
761770
EXPECT_DEATH(formatv("{}{1}", 0, 1).str(),
762771
"Cannot mix automatic and explicit indices");
772+
EXPECT_DEATH(formatv("{}{0}{}", 0, 1).str(),
773+
"Cannot mix automatic and explicit indices");
763774
#else // GTEST_HAS_DEATH_TEST
764775
GTEST_SKIP() << "No support for EXPECT_DEATH";
765776
#endif // GTEST_HAS_DEATH_TEST
@@ -768,6 +779,7 @@ TEST(FormatVariadicTest, Validate) {
768779
EXPECT_EQ(formatv("{0}", 1, 2).str(), "1");
769780
EXPECT_EQ(formatv("{0} {2}", 1, 2, 3).str(), "1 3");
770781
EXPECT_EQ(formatv("{}{1}", 0, 1).str(), "01");
782+
EXPECT_EQ(formatv("{}{0}{}", 0, 1).str(), "001");
771783
#endif // NDEBUG
772784
}
773785

0 commit comments

Comments
 (0)