This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by yvvan on Jun 18 2019, 3:12 AM.

Details

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"

Diff Detail

Repository
rL LLVM

Event Timeline

yvvan created this revision.Jun 18 2019, 3:12 AM

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 updated this revision to Diff 207062.Jun 28 2019, 7:23 AM

Sorry for delay.
Test added, redundant comments removed.

gribozavr added inline comments.Jul 1 2019, 7:15 AM
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 marked an inline comment as done.Jul 1 2019, 10:56 PM
yvvan added inline comments.
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 added inline comments.Jul 1 2019, 11:02 PM
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 marked an inline comment as done.Jul 2 2019, 12:32 AM
yvvan added inline comments.
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.

yvvan updated this revision to Diff 207483.Jul 2 2019, 12:39 AM
gribozavr accepted this revision.Jul 2 2019, 12:54 AM

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

This revision is now accepted and ready to land.Jul 2 2019, 12:54 AM
yvvan added a comment.Jul 3 2019, 1:22 AM

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?

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 added a comment.Jul 3 2019, 2:09 AM

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 added a comment.Jul 3 2019, 2:10 AM
In D63482#1567927, @nik 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 added a comment.Jul 3 2019, 2:51 AM

This script does not seem to work properly on windows.

yvvan added a comment.Jul 3 2019, 3:10 AM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 3:21 AM
mgehre added a subscriber: mgehre.Apr 30 2020, 2:14 PM

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 added a comment.May 4 2020, 2:04 AM

@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 added a comment.May 4 2020, 4:04 AM

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 added a comment.May 6 2020, 1:00 AM

@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 added a subscriber: DmitryPolukhin.EditedMay 20 2020, 6:23 AM

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

yvvan added a comment.Jun 3 2020, 11:29 PM

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