This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jurahul on Jul 15 2020, 10:26 AM.

Details

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

Diff Detail

Event Timeline

jurahul created this revision.Jul 15 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2020, 10:26 AM
jurahul edited the summary of this revision. (Show Details)Jul 15 2020, 10:32 AM
jurahul edited the summary of this revision. (Show Details)
sammccall accepted this revision.Jul 20 2020, 6:51 AM

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

This revision is now accepted and ready to land.Jul 20 2020, 6:51 AM
jurahul updated this revision to Diff 279250.Jul 20 2020, 8:04 AM

Review feedback

jurahul edited the summary of this revision. (Show Details)Jul 20 2020, 8:05 AM
jurahul marked 4 inline comments as done.
jurahul added inline comments.
llvm/lib/Support/FormatVariadic.cpp
94

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

This revision was automatically updated to reflect the committed changes.
jurahul marked an inline comment as done.