Page MenuHomePhabricator

run-clang-tidy: forward clang-tidy exit status
ClosedPublic

Authored by vmiklos on Mar 11 2018, 7:31 AM.

Details

Summary

Exit with a non-zero value in case any of the underlying clang-tidy
invocations exit with a non-zero value.

This is useful in case WarningsAsErrors is enabled for some of the
checks: if any of those checks find something, the exit status now
reflects that.

Also add the ability to use run-clang-tidy.py via lit, and assert that
the exit code is not 0 when modernize-use-auto is triggered
intentionally.

Diff Detail

Event Timeline

vmiklos created this revision.Mar 11 2018, 7:31 AM

This looks reasonable to me, but is there a way to test it?

I grepped for run-clang-tidy under tools/clang/tools/extra/test/clang-tidy/, but found nothing, so I assume run-clang-tidy has no test coverage at the moment.

Do we have some other test that requires a compilation database, so I could write a test similar to that one for the run-clang-tidy.py case? I expect it's a bit tricky, since the compilation database will want absolute paths.

aaron.ballman accepted this revision.Mar 12 2018, 7:21 AM

Yeah, I cannot find any test coverage for this either. I think this is safe to commit without test coverage, but you should wait to see if @alexfh has ideas on testing before committing.

This revision is now accepted and ready to land.Mar 12 2018, 7:21 AM

I would appreciate someone adding a test for this script, but I can understand if you say you have no time for this.

clang-tidy/tool/run-clang-tidy.py
167

errors is a misleading name here. It seems to be a list of file names that clang-tidy failed on? Could be failed_files or something like that.

vmiklos updated this revision to Diff 138165.Mar 13 2018, 5:53 AM

I've went with failed_files. And yes, this could be a simple reference, I just exploit the fact here that a list is passed by reference in Python.

I would appreciate someone adding a test for this script, but I can understand if you say you have no time for this.

I can look into that, I just did not find any other test that works with a compilation database (so I can write a run-clang-tidy test similar to that).

I would appreciate someone adding a test for this script, but I can understand if you say you have no time for this.

I can look into that, I just did not find any other test that works with a compilation database (so I can write a run-clang-tidy test similar to that).

Most of the tests that use a compilation database are located under llvm/tools/clang/test/Tooling/. These tests create a directory with source file(s) and a corresponding compile_commands.json file and then just run the tool on these file(s).

Most of the tests that use a compilation database are located under llvm/tools/clang/test/Tooling/. These tests create a directory with source file(s) and a corresponding compile_commands.json file and then just run the tool on these file(s).

Thanks, the other missing bit was how to find the path to run-clang-tidy.py, but now I found that tools/clang/tools/extra/test/lit.cfg can be extended to recognize a %run_clang_tidy.

vmiklos updated this revision to Diff 138812.Mar 17 2018, 5:57 AM
vmiklos edited the summary of this revision. (Show Details)

Updated patch now includes the first run-clang-tidy.py testcase. :-)

aaron.ballman accepted this revision.Mar 17 2018, 8:24 AM

This looks reasonable to me, but I'm also not a python expert.

This revision was automatically updated to reflect the committed changes.