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

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: llvm/include/llvm/Support/FormatVariadic.h:208
 // 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.
----------------
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 "{{".`?


================
Comment at: llvm/lib/Support/FormatVariadic.cpp:94
 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.
----------------
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.


================
Comment at: llvm/lib/Support/FormatVariadic.cpp:106
       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);
----------------
nit: take_front


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83888/new/

https://reviews.llvm.org/D83888