[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

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

nit: take_front

[jurahul]

Review feedback

[jurahul]

llvm/lib/Support/FormatVariadic.cpp

94

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 -- "{{" to print a literal '{'. In order to
+// print a literal '}', it should not be doubled.
 //
 // ===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/lib/Support/FormatVariadic.cpp

if (!RepString.empty()) {
     assert(false && "Unexpected characters found in replacement string!");
   }
 
   return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
 }
 
 std::pair<ReplacementItem, StringRef>
 formatv_object_base::splitLiteralAndReplacement(StringRef Fmt) {
-  std::size_t From = 0;
-  while (From < Fmt.size() && From != StringRef::npos) {
-    std::size_t BO = Fmt.find_first_of('{', From);
+  while (!Fmt.empty()) {
     // Everything up until the first brace is a literal.
-    if (BO != 0)
+    if (Fmt.front() != '{') {
+      std::size_t BO = Fmt.find_first_of('{');
       return std::make_pair(ReplacementItem{Fmt.substr(0, BO)}, Fmt.substr(BO));
+    }
 
-    StringRef Braces =
-        Fmt.drop_front(BO).take_while([](char C) { return C == '{'; });
+    StringRef Braces = Fmt.take_while([](char C) { return C == '{'; });
     // If there is more than one brace, then some of them are escaped.  Treat
     // these as replacements.
     if (Braces.size() > 1) {
       size_t NumEscapedBraces = Braces.size() / 2;
-      StringRef Middle = Fmt.substr(BO, NumEscapedBraces);
-      StringRef Right = Fmt.drop_front(BO + NumEscapedBraces * 2);
+      StringRef Middle = Fmt.substr(0, NumEscapedBraces);
+      StringRef Right = Fmt.drop_front(NumEscapedBraces * 2);
       return std::make_pair(ReplacementItem{Middle}, Right);
     }
     // An unterminated open brace is undefined.  We treat the rest of the string
     // as a literal replacement, but we assert to indicate that this is
     // undefined and that we consider it an error.
-    std::size_t BC = Fmt.find_first_of('}', BO);
+    std::size_t BC = Fmt.find_first_of('}');
     if (BC == StringRef::npos) {
       assert(
           false &&
           "Unterminated brace sequence.  Escape with {{ for a literal brace.");
       return std::make_pair(ReplacementItem{Fmt}, StringRef());
     }
 
     // Even if there is a closing brace, if there is another open brace before
     // this closing brace, treat this portion as literal, and try again with the
     // next one.
-    std::size_t BO2 = Fmt.find_first_of('{', BO + 1);
+    std::size_t BO2 = Fmt.find_first_of('{', 1);
     if (BO2 < BC)
       return std::make_pair(ReplacementItem{Fmt.substr(0, BO2)},
                             Fmt.substr(BO2));
 
-    StringRef Spec = Fmt.slice(BO + 1, BC);
+    StringRef Spec = Fmt.slice(1, BC);
     StringRef Right = Fmt.substr(BC + 1);
 
     auto RI = parseReplacementItem(Spec);
     if (RI.hasValue())
       return std::make_pair(*RI, Right);
 
     // If there was an error parsing the replacement item, treat it as an
     // invalid replacement spec, and just continue.
-    From = BC + 1;
+    Fmt = Fmt.drop_front(BC + 1);
   }
   return std::make_pair(ReplacementItem{Fmt}, StringRef());
 }
 
 SmallVector<ReplacementItem, 2>
 formatv_object_base::parseFormatString(StringRef Fmt) {
   SmallVector<ReplacementItem, 2> Replacements;
   ReplacementItem I;

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);