Page MenuHomePhabricator

[clang-tidy] Integrate clang-tidy-diff.py machinery into run-clang-tidy.py
Needs ReviewPublic

Authored by zinovy.nis on Mar 16 2019, 3:15 AM.

Details

Summary

This patch adds a new --diff[=2] option to run-clang-tidy.py to run clang-tidy checks against the given diff only, instead of the whole file.

Usage:

git show -U2 | /some/path/1/run-clang-tidy.py --diff=1 -clang-tidy-binary /some/path/2/clang-tidy -p out/Debug -checks=-*,performance-* option_regex

Diff Detail

Event Timeline

zinovy.nis created this revision.Mar 16 2019, 3:15 AM
zinovy.nis edited the summary of this revision. (Show Details)Mar 16 2019, 3:19 AM

How is this functionality different from the clang-tidy-diff.py script with the -j option being added in the other patch?

How is this functionality different from the clang-tidy-diff.py script with the -j option being added in the other patch?

It's the same.

Can we delete clang-tidy-diff.py with this? (Or delete most of its contents and make it forward to this script?)

zinovy.nis added a comment.EditedMar 28 2019, 11:27 PM

Can we delete clang-tidy-diff.py with this? (Or delete most of its contents and make it forward to this script?)

I don't mind deleting clang-tidy-diff.py, but someone may want to use it for some legacy reasons. AFAIK @alexfh says they are using it.

Or delete most of its contents and make it forward to this script

IMO it's a bad idea as these scripts are completely standalone now and it is useful.

Added a test.

alexfh added a comment.Apr 1 2019, 4:45 AM

How is this functionality different from the clang-tidy-diff.py script with the -j option being added in the other patch?

It's the same.

Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a distinct and somewhat self-documenting name and a very specific purpose (find clang-tidy regressions in a patch), while run-clang-tidy.py is more focused on running over larger bodies of code with a purpose of analyzing or cleaning up. Is there any benefit in having all functionality in run-clang-tidy.py?

Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a distinct and somewhat self-documenting name and a very specific purpose (find clang-tidy regressions in a patch), while run-clang-tidy.py is more focused on running over larger bodies of code with a purpose of analyzing or cleaning up. Is there any benefit in having all functionality in run-clang-tidy.py?

Both scripts have almost the same implementation, almost the same syntax (at least after -j and -timeout we introduced). So why not merge them.

@alexfh, do you still have any objections?

alexfh added a comment.Apr 3 2019, 7:48 AM

Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a distinct and somewhat self-documenting name and a very specific purpose (find clang-tidy regressions in a patch), while run-clang-tidy.py is more focused on running over larger bodies of code with a purpose of analyzing or cleaning up. Is there any benefit in having all functionality in run-clang-tidy.py?

Both scripts have almost the same implementation, almost the same syntax (at least after -j and -timeout we introduced). So why not merge them.

There are multiple (mostly user-facing) reasons to prefer separate scripts. Simplicity, focus, and internal consistency come to mind first. The clang-tidy-diff being separate and serving a single purpose makes it easier to find, understand, and use. If we merge the two scripts together, will it be easy to understand the different use-cases? Will all options be compatible? (E.g. -header-filter doesn't make much sense for the --diff mode, since the diff defines a different filter)