Page MenuHomePhabricator

[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