[PATCH] D76037: [clang] tooling replacements are escaped when exporting to YAML

[Nathan James via Phabricator via cfe-commits]

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