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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

Sent D80301  for review.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@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 \n\n'` is actually replacement text with two new lines in 
standard YAML deserialiser.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@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"


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365017: [clang-tidy] Fix the YAML created for checks like 
modernize-pass-by-value (authored by yvvan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63482?vs=207483=207747#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482

Files:
  cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
  cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp


Index: cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
+++ cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 
\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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
===
--- cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
+++ cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,13 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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,


Index: cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
+++ cfe/trunk/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 \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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
===
--- cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
+++ cfe/trunk/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,13 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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 = 

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

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


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

This script does not seem to work properly on windows.


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

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


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

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)


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

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


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

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?


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added a comment.
This revision is now accepted and ready to land.

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


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan updated this revision to Diff 207483.

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

https://reviews.llvm.org/D63482

Files:
  clang/include/clang/Tooling/ReplacementsYaml.h
  clang/unittests/Tooling/ReplacementsYamlTest.cpp


Index: clang/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- clang/unittests/Tooling/ReplacementsYamlTest.cpp
+++ clang/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 
\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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,13 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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,


Index: clang/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- clang/unittests/Tooling/ReplacementsYamlTest.cpp
+++ clang/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 \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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,13 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan marked an inline comment as done.
yvvan added inline comments.



Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {

gribozavr wrote:
> yvvan wrote:
> > gribozavr wrote:
> > > 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...
> > 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.
> What I'm saying that in this constructor RelpcamentText is an empty string, 
> always, and therefore it does not contain a \n.
Ah, yes, my fault. I've generated the wrong diff. Thanks, will update it soon.


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {

yvvan wrote:
> gribozavr wrote:
> > 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...
> 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.
What I'm saying that in this constructor RelpcamentText is an empty string, 
always, and therefore it does not contain a \n.


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan marked an inline comment as done.
yvvan added inline comments.



Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {

gribozavr wrote:
> 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...
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.


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:35
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {

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...


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan updated this revision to Diff 207062.
yvvan added a comment.

Sorry for delay.
Test added, redundant comments removed.


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

https://reviews.llvm.org/D63482

Files:
  clang/include/clang/Tooling/ReplacementsYaml.h
  clang/unittests/Tooling/ReplacementsYamlTest.cpp


Index: clang/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- clang/unittests/Tooling/ReplacementsYamlTest.cpp
+++ clang/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 
\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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -31,7 +31,13 @@
   /// access to its data members.
   struct NormalizedReplacement {
 NormalizedReplacement(const IO &)
-: FilePath(""), Offset(0), Length(0), ReplacementText("") {}
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {
+ReplacementText.replace(lineBreakPos, 1, "\n\n");
+lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+  }
+}
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : FilePath(R.getFilePath()), Offset(R.getOffset()),


Index: clang/unittests/Tooling/ReplacementsYamlTest.cpp
===
--- clang/unittests/Tooling/ReplacementsYamlTest.cpp
+++ clang/unittests/Tooling/ReplacementsYamlTest.cpp
@@ -46,6 +46,30 @@
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 \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 \n\n'\n"
+   "...\n",
+   YamlContentStream.str().c_str());
+}
+
 TEST(ReplacementsYamlTest, deserializesReplacements) {
   std::string YamlContent = "---\n"
 "MainSourceFile:  /path/to/source.cpp\n"
Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -31,7 +31,13 @@
   /// access to its data members.
   struct NormalizedReplacement {
 NormalizedReplacement(const IO &)
-: FilePath(""), Offset(0), Length(0), ReplacementText("") {}
+: FilePath(""), Offset(0), Length(0), ReplacementText("") {
+  size_t lineBreakPos = ReplacementText.find('\n');
+  while (lineBreakPos != std::string::npos) {
+ReplacementText.replace(lineBreakPos, 1, "\n\n");
+lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+  }
+}
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : FilePath(R.getFilePath()), Offset(R.getOffset()),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

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




Comment at: clang/include/clang/Tooling/ReplacementsYaml.h:43
+ReplacementText.replace(lineBreakPos, 1, "\n\n");
+// Get the next occurrence from the current position
+lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);

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


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

https://reviews.llvm.org/D63482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan created this revision.
yvvan added reviewers: gribozavr, nik.
Herald added a subscriber: xazax.hun.

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 
 " instead of "#include \n"


https://reviews.llvm.org/D63482

Files:
  clang/include/clang/Tooling/ReplacementsYaml.h


Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,15 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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) {
+// Replace this occurrence of Sub String
+ReplacementText.replace(lineBreakPos, 1, "\n\n");
+// Get the next occurrence from the current position
+lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+  }
+}
 
 clang::tooling::Replacement denormalize(const IO &) {
   return clang::tooling::Replacement(FilePath, Offset, Length,


Index: clang/include/clang/Tooling/ReplacementsYaml.h
===
--- clang/include/clang/Tooling/ReplacementsYaml.h
+++ clang/include/clang/Tooling/ReplacementsYaml.h
@@ -35,7 +35,15 @@
 
 NormalizedReplacement(const IO &, const clang::tooling::Replacement )
 : 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) {
+// Replace this occurrence of Sub String
+ReplacementText.replace(lineBreakPos, 1, "\n\n");
+// Get the next occurrence from the current position
+lineBreakPos = ReplacementText.find('\n', lineBreakPos + 2);
+  }
+}
 
 clang::tooling::Replacement denormalize(const IO &) {
   return clang::tooling::Replacement(FilePath, Offset, Length,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits