Page MenuHomePhabricator

Add option to export fixes to run-clang-tidy.py
ClosedPublic

Authored by mfherbst on Mar 24 2017, 5:05 AM.

Details

Summary

This patch adds the option to keep the list of proposed fixes even though they should not be applied.

This allows to detect possible fixes using the parallelised run-clang-tidy.py and apply them using clang-apply-replacements at a later time.
Essentially the patch causes the individual temporary yaml files by the parallel clang-tidy instances to be merged into one user-defined file.

Diff Detail

Repository
rL LLVM

Event Timeline

mfherbst created this revision.Mar 24 2017, 5:05 AM
malcolm.parsons edited reviewers, added: aaron.ballman, alexfh; removed: Restricted Project.Mar 24 2017, 10:45 AM
malcolm.parsons added a subscriber: cfe-commits.
mfherbst updated this revision to Diff 92986.Mar 24 2017, 11:31 AM

Fixed typos (MainFile => MainSourceFile)

alexfh requested changes to this revision.Jul 5 2017, 7:48 AM
alexfh added inline comments.
run-clang-tidy.py
49 ↗(On Diff #92986)

Please sort the imports

99 ↗(On Diff #92986)

Please put continue on the next line.

105 ↗(On Diff #92986)

This is going to be the case for each non-trivial invocation of this script. Do we need to keep MainSourceFile at all?

242 ↗(On Diff #92986)

nit: Add a space after the comma.

This revision now requires changes to proceed.Jul 5 2017, 7:48 AM
mfherbst updated this revision to Diff 105592.Jul 7 2017, 1:07 AM
mfherbst edited edge metadata.
  • Adapted patch to changes suggested by alexfh
  • Added comment why setting MainSourceFile to an empty string if fixes are exported
mfherbst added inline comments.Jul 7 2017, 1:10 AM
run-clang-tidy.py
105 ↗(On Diff #92986)

Good point. As it turns out clang-apply-replacement never uses the value afaik. The field needs to be around, however, since the definition of the replacements `yaml format ( see clang/include/clang/Tooling/ReplacementsYaml.h`) makes it mandatory.

alexfh requested changes to this revision.Jul 7 2017, 6:47 AM

A few more nits.

run-clang-tidy.py
93 ↗(On Diff #105592)

I'm not sure "fixfile" is a word. Just "file" maybe?

96 ↗(On Diff #105592)

nit: "usid"

100 ↗(On Diff #105592)

I'd use os.path.join() instead of concatenation.

104 ↗(On Diff #105592)

nit: Add a trailing period.

109 ↗(On Diff #105592)

nit: Add a trailing period.

This revision now requires changes to proceed.Jul 7 2017, 6:47 AM
mfherbst updated this revision to Diff 105818.Jul 10 2017, 1:22 AM
mfherbst edited edge metadata.

Correct nits and typos.

mfherbst added inline comments.Jul 10 2017, 1:25 AM
run-clang-tidy.py
93 ↗(On Diff #105592)

I thought I would refer to the name of the input argument, but I guess you are right, file makes it less clunky to read.

alexfh accepted this revision.Jul 10 2017, 2:16 AM

Just noticed another couple of nits, otherwise looks good. Thanks!

Do you need me to commit the patch for you?

clang-tidy/tool/run-clang-tidy.py
158 ↗(On Diff #105818)

nit: Add a trailing period.

240 ↗(On Diff #105818)

nit: Add + '...' at the end for consistency with other similar messages.

This revision is now accepted and ready to land.Jul 10 2017, 2:16 AM
mfherbst updated this revision to Diff 105835.Jul 10 2017, 3:49 AM

Correct the nits suggested by alexfh

I do not have commit rights to the svn if that's what you're asking, but I could send the patch to llvm-commits if that makes it easier for you.

I do not have commit rights to the svn if that's what you're asking, but I could send the patch to llvm-commits if that makes it easier for you.

No need to send the patch to the list separately, Phabricator is rather convenient for this purpose. The only issue is that I can't seem to be able to apply the patch cleanly. Could you rebase your changes on top of HEAD and regenerate the patch with the full context (as documented on http://llvm.org/docs/Phabricator.html).

mfherbst updated this revision to Diff 107254.Jul 18 2017, 11:24 PM

Adapt patch to the changes since the initial submission.

Most notably the introduction of clang-tidy-4.0 changed the format in which clang-tidy dumps the required changes. This needs to be taken into account when merging the suggested fixes. This patch is now adapted such that it can deal with both the old and the new format at the same time.

Please review the patch once again as it differs quite a bit from the initial submission.

alexfh requested changes to this revision.Jul 20 2017, 6:29 AM
alexfh added inline comments.
clang-tidy/tool/run-clang-tidy.py
96–98 ↗(On Diff #107254)

I don't think there's much sense in supporting the old format, since the script is distributed together with the binary, so it's unlikely that it will encounter clang-tidy of an earlier version.

This revision now requires changes to proceed.Jul 20 2017, 6:29 AM
mfherbst updated this revision to Diff 107633.Jul 20 2017, 10:58 PM
mfherbst edited edge metadata.

Remove compatiblity with clang-tidy < 4.0.0

alexfh accepted this revision.Jul 21 2017, 2:56 AM

LG. I'll commit the patch for you

This revision is now accepted and ready to land.Jul 21 2017, 2:56 AM

Many thanks!

This revision was automatically updated to reflect the committed changes.

Committed as r308726.

Thank you for the contribution!

One note for the future: the patch doesn't contain repository-based paths, so I had to apply it manually. There's a good documentation at http://llvm.org/docs/Phabricator.html, which you could follow to create the patch next time. Thanks!