njames93 added inline comments.
================
Comment at: clang/lib/Tooling/ReplacementsYaml.cpp:22
+static constexpr Escapes EscapeChars[] = {
+ {'\n', 'n'}, {'\r', 'r'}, {'\t', 't'}, {'\\', '\\'}};
+
----------------
AlexanderLanin wrote:
> Just so I have asked ;-)
> Escaping every \ would be incorrect? Basically duplicate every '\'.
You need to escape the escape character to avoid ambiguity when unescaping later
Say the code has the raw string which is `\` followed by `n`. it would be
escaped as `\n`.
Then when it comes to unescape it will read `\n` as a newline.
Escaping the `\` leads to the output being `\\n` which will be read back as a
`\` followed by `n`.
================
Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:55
+ Doc.Replacements.emplace_back(FilePath, 0, 0, "#include <utility>\n");
+ Doc.Replacements.emplace_back(FilePath, 0, 0, "'\\\t\r\n");
----------------
AlexanderLanin wrote:
> I think it would be worthwhile to test other characters as well.
> 50% of that would be purely for documentation purposes. What would happen
> when you escape \x and unescape \\x?
I have added all the c++ escape characters apart from `\'` as that is handled
in the yaml string parser anyway and `\"` as that seems to be ignore anyway
================
Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:76
+ ASSERT_EQ(DocR->getOffset(), NewDocR->getOffset());
+ ASSERT_EQ(DocR->getReplacementText(), NewDocR->getReplacementText());
+ }
----------------
AlexanderLanin wrote:
> I assume this kind of test would have been green even without your change? Or
> would it fail?
> You are testing that it is reconstructed correctly (which is indeed the main
> point), but not the escaping and unescaping.
> You should probably test a concrete example with(escaped text, expected
> escaped test).
Yes this should've passed before however the issue was one more of readability.
Things like the old double newline just look confusing whereas every programmer
knows that '\n' is code for newline.
I have planned to add that in.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76037/new/
https://reviews.llvm.org/D76037
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
