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

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin requested changes to this revision.
AlexanderLanin added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/Tooling/ReplacementsYaml.cpp:22
+static constexpr Escapes EscapeChars[] = {
+    {'\n', 'n'}, {'\r', 'r'}, {'\t', 't'}, {'\\', '\\'}};
+
----------------
Just so I have asked ;-)
Escaping every \ would be incorrect? Basically duplicate every '\'.


================
Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:68
+  ASSERT_EQ(Doc.Replacements.size(), NewDoc.Replacements.size());
+  if (Doc.Replacements.size() == NewDoc.Replacements.size()) {
+    for (auto DocR = Doc.Replacements.begin(),
----------------
njames93 wrote:
> Can this ever be false. Does execution of a test case stop once an ASSERT_EQ fails
Yes, assert stops the test case. After the assert you can safely assume they are identical.


================
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");
 
----------------
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?


================
Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:76
+      ASSERT_EQ(DocR->getOffset(), NewDocR->getOffset());
+      ASSERT_EQ(DocR->getReplacementText(), NewDocR->getReplacementText());
+    }
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76037