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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
