[LLVM] Update formatv() documentation to clarify no escape for `}`

[jurahul]

Summary

• Update documentation to clarify that } does not need to be doubled up.
• Update EscapedBrace test case to test this behavior

[sammccall]

Thanks, this seems much easier than changing the behavior for existing callers, and much better than having two different behaviors.

llvm/include/llvm/Support/FormatVariadic.h

208

This phrasing seems a little awkward. Maybe just: A literal '}' need not be doubled.?

There is also some redundancy now in '{' it must be doubled -- "{{" to print a literal '{'.
What about ... to print a literal '{' it must be doubled as "{{".?

llvm/lib/Support/FormatVariadic.cpp

94 ↗

(On Diff #278240)

this simplification seems fine to me, but it does seem confusing to combine a change in contract with a change in implementation but no change in behavior.

I'd suggest making these two separate patches, but up to you.

106 ↗

(On Diff #278240)

nit: take_front

[jurahul]

Review feedback

[jurahul]

llvm/lib/Support/FormatVariadic.cpp

94 ↗

(On Diff #278240)

That makes sense. I'll start a new review for changes in this file.

llvm/include/llvm/Support/FormatVariadic.h

 //           trailing padding.  If this is greater than the required length,
 //           then the text is output according to the value of `loc`, and padded
 //           as appropriate on the left and/or right by `char`.
 //
 // ===Special Characters===
 //
 // The characters '{' and '}' are reserved and cannot appear anywhere within a
 // replacement sequence.  Outside of a replacement sequence, in order to print
-// a literal '{' or '}' it must be doubled -- "{{" to print a literal '{' and
-// "}}" to print a literal '}'.
+// a literal '{' it must be doubled as "{{".
 //
 // ===Parameter Indexing===
+//
 // `index` specifies the index of the parameter in the parameter pack to format
 // into the output.  Note that it is possible to refer to the same parameter
 // index multiple times in a given format string.  This makes it possible to
 // output the same value multiple times without passing it multiple times to the
 // function. For example:
 //
 //   formatv("{0} {1} {0}", "a", "bb")
 //

llvm/unittests/Support/FormatVariadicTest.cpp

TEST(FormatVariadicTest, EscapedBrace) {
   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 = 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 = 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 = formatv_object_base::parseFormatString("}}}");
+  ASSERT_EQ(1u, Replacements.size());
+  EXPECT_EQ("}}}", Replacements[0].Spec);
+  EXPECT_EQ(ReplacementType::Literal, Replacements[0].Type);
 }
 
 TEST(FormatVariadicTest, ValidReplacementSequence) {
   // 1. Simple replacement - parameter index only
   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);