[clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

[yvvan]

Summary

Currently this check generates the replacement with the newline in the end. The proper way to export it to YAML is to have two \n\n instead of one.

Without this fix clients should reinterpret the replacement as "#include <utility> " instead of "#include <utility>\n"

[gribozavr]

Thanks! Please add tests in ./unittests/Tooling/ReplacementsYamlTest.cpp.

clang/include/clang/Tooling/ReplacementsYaml.h

43 ↗

(On Diff #205289)

Please add comments that explain why this is the correct serialization. Please don't add comments that repeat the code.

[yvvan]

Sorry for delay.
Test added, redundant comments removed.

[gribozavr]

clang/include/clang/Tooling/ReplacementsYaml.h

35 ↗

(On Diff #207062)

Sorry, I don't understand how this works -- ReplacementText does not contain a \n, so lineBreakPos will be npos, and the loop below will not execute...

[yvvan]

clang/include/clang/Tooling/ReplacementsYaml.h

35 ↗

(On Diff #207062)

Quite opposite. This patch targets the cases where it's not npos (see the test example). So it has a line break and this line break should be transformed into two line breaks.

[gribozavr]

clang/include/clang/Tooling/ReplacementsYaml.h

35 ↗

(On Diff #207062)

What I'm saying that in this constructor RelpcamentText is an empty string, always, and therefore it does not contain a \n.

[yvvan]

clang/include/clang/Tooling/ReplacementsYaml.h

35 ↗

(On Diff #207062)

Ah, yes, my fault. I've generated the wrong diff. Thanks, will update it soon.

[gribozavr]

Thanks! Do you have commit access? Would you like me to commit this patch for you?

[yvvan]

I have a commit access but I don't understand how am I supposed to commit (haven't done that for a while). There's no clang svn repo anymore. Do you know what's the current state of repositories and where to commit?

[gribozavr]

SVN repo is still there.

However, I don't know how to commit using SVN, I commit using the git-svn integration (which still commits using the "svn" command under the hood, but as a user you will be interacting with a git repo).

git clone https://github.com/llvm/llvm-project.git
cd llvm-project
... make changes...
cd llvm
./utils/git-svn/git-llvm push

[nik]

In D63482#1567897, @yvvan wrote:

I have a commit access but I don't understand how am I supposed to commit (haven't done that for a while). There's no clang svn repo anymore. Do you know what's the current state of repositories and where to commit?

It's fairly simple with git nowadays: https://llvm.org/docs/GettingStarted.html#id15
(at least with mono repo)

[nik]

In D63482#1567927, @nik wrote:

In D63482#1567897, @yvvan wrote:

I have a commit access but I don't understand how am I supposed to commit (haven't done that for a while). There's no clang svn repo anymore. Do you know what's the current state of repositories and where to commit?

It's fairly simple with git nowadays: https://llvm.org/docs/GettingStarted.html#id15
(at least with mono repo)

Ops, correct I've meant https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git

[yvvan]

This script does not seem to work properly on windows.

[yvvan]

ok, will do it through svn. i forgot that clang repo is called "cfe" so it's there

[mgehre]

This change breaks modernize-use-using fixits.
The code
typedef int A, B;
is replaced into

using A = int;
using B = int;

when directly applying fixits with clang-tidy. But it is replaced into

using A = int;

using B = int;

when applying fixits with clang-apply-replacements.
This is because the check creates two replacements,
the first one being typedef int A -> using A = int
and the second one being , B -> ;\nusing B = int.

With this change, the newline is duplicated, which introduces an unwanted newline.
Any advice how to fix this properly?

[yvvan]

@mgehre From your comment it seems that clang-apply-replacements handles the YAML wrong and does not make the proper conversion back from "\n\n" to "\n"

[mgehre]

Could be - did it handle it correctly for your include case here?

And if this is a general yaml string thing, shouldn't the replacement of newlines (for both ways) happen in the yaml parser/writer instead of clang/Tooling?

[yvvan]

@mgehre I think we need to adjust denormalize(const IO &) method here to convert \n back properly. It seems I missed it in my patch.

[DmitryPolukhin]

@mgehre @yvvan it seems that the issue still not fixed and '\n' gets duplicated in the replacements. Are you going to fix this issue or I should create a patch to fix it?
Before this change '\n' was actually processed correctly ReplacementText: '#include <utility>\n\n' is actually replacement text with two new lines in standard YAML deserialiser.

[DmitryPolukhin]

Sent D80301 for review.

[yvvan]

@DmitryPolukhin Sorry, I didn't have time recently. Thanks a lot for taking care!

cfe/trunk/include/clang/Tooling/ReplacementsYaml.h

template <> struct MappingTraits<clang::tooling::Replacement> {
   /// Helper to (de)serialize a Replacement since we don't have direct
   /// access to its data members.
   struct NormalizedReplacement {
     NormalizedReplacement(const IO &)
         : FilePath(""), Offset(0), Length(0), ReplacementText("") {}
 
     NormalizedReplacement(const IO &, const clang::tooling::Replacement &R)
         : FilePath(R.getFilePath()), Offset(R.getOffset()),
-          Length(R.getLength()), ReplacementText(R.getReplacementText()) {}
+          Length(R.getLength()), ReplacementText(R.getReplacementText()) {
+      size_t lineBreakPos = ReplacementText.find('\n');
+      while (lineBreakPos != std::string::npos) {
+        ReplacementText.replace(lineBreakPos, 1, "\n\n");
+        lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+      }
+    }
 
     clang::tooling::Replacement denormalize(const IO &) {
       return clang::tooling::Replacement(FilePath, Offset, Length,
                                          ReplacementText);
     }
 
     std::string FilePath;
     unsigned int Offset;

cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp

ASSERT_STREQ("---\n"
                "  - FilePath:        '/path/to/file2.h'\n"
                "    Offset:          301\n"
                "    Length:          2\n"
                "    ReplacementText: 'replacement #2'\n"
                "...\n",
                YamlContentStream.str().c_str());
 }
 
+TEST(ReplacementsYamlTest, serializesNewLines) {
+  TranslationUnitReplacements Doc;
+
+  Doc.MainSourceFile = "/path/to/source.cpp";
+  Doc.Replacements.emplace_back("/path/to/file1.h", 0, 0, "#include <utility>\n");
+
+  std::string YamlContent;
+  llvm::raw_string_ostream YamlContentStream(YamlContent);
+
+  yaml::Output YAML(YamlContentStream);
+  YAML << Doc;
+
+  // NOTE: If this test starts to fail for no obvious reason, check whitespace.
+  ASSERT_STREQ("---\n"
+               "MainSourceFile:  '/path/to/source.cpp'\n"
+               "Replacements:    \n" // Extra whitespace here!
+               "  - FilePath:        '/path/to/file1.h'\n"
+               "    Offset:          0\n"
+               "    Length:          0\n"
+               "    ReplacementText: '#include <utility>\n\n'\n"
+               "...\n",
+               YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
                             "MainSourceFile:      /path/to/source.cpp\n"
                             "Replacements:\n"
                             "  - FilePath:        /path/to/file1.h\n"
                             "    Offset:          232\n"
                             "    Length:          56\n"
                             "    ReplacementText: 'replacement #1'\n"