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

[vmiklos]

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.

[aaron.ballman]

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

[vmiklos]

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]

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.

[alexfh]

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]

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

[alexfh]

In D44366#1035804, @vmiklos wrote:

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

[vmiklos]

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 patch now includes the first run-clang-tidy.py testcase. :-)

[aaron.ballman]

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

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

def apply_fixes(args, tmpdir):
   if args.format:
     invocation.append('-format')
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue):
+def run_tidy(args, tmpdir, build_path, queue, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
     name = queue.get()
     invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
                                      args.extra_arg, args.extra_arg_before,
                                      args.quiet, args.config)
     sys.stdout.write(' '.join(invocation) + '\n')
-    subprocess.call(invocation)
+    return_code = subprocess.call(invocation)
+    if return_code != 0:
+      failed_files.append(name)
     queue.task_done()
 
 
 def main():
   parser = argparse.ArgumentParser(description='Runs clang-tidy over all files '
                                    'in a compilation database. Requires '
                                    'clang-tidy and clang-apply-replacements in '
                                    '$PATH.')
def main():
   tmpdir = None
   if args.fix or args.export_fixes:
     check_clang_apply_replacements_binary(args)
     tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
   file_name_re = re.compile('|'.join(args.files))
 
+  return_code = 0
   try:
     # Spin up a bunch of tidy-launching threads.
     task_queue = queue.Queue(max_task)
+    # List of files with a non-zero return code.
+    failed_files = []
     for _ in range(max_task):
       t = threading.Thread(target=run_tidy,
-                           args=(args, tmpdir, build_path, task_queue))
+                           args=(args, tmpdir, build_path, task_queue, failed_files))
       t.daemon = True
       t.start()
 
     # Fill the queue with files.
     for name in files:
       if file_name_re.search(name):
         task_queue.put(name)
 
     # Wait for all threads to be done.
     task_queue.join()
+    if len(failed_files):
+      return_code = 1
 
   except KeyboardInterrupt:
     # This is a sad hack. Unfortunately subprocess goes
     # bonkers with ctrl-c and we start forking merrily.
     print('\nCtrl-C detected, goodbye.')
     if tmpdir:
       shutil.rmtree(tmpdir)
     os.kill(0, 9)
 
-  return_code = 0
   if args.export_fixes:
     print('Writing fixes to ' + args.export_fixes + ' ...')
     try:
       merge_replacement_files(tmpdir, args.export_fixes)
     except:
       print('Error exporting fixes.\n', file=sys.stderr)
       traceback.print_exc()
       return_code=1

test/clang-tidy/run-clang-tidy.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\/\\\\/g' > %t/compile_commands.json
+// RUN: echo "Checks: '-*,modernize-use-auto'" > %t/.clang-tidy
+// RUN: echo "WarningsAsErrors: '*'" >> %t/.clang-tidy
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: cd "%t"
+// RUN: not %run_clang_tidy "%t/test.cpp"
+
+int main()
+{
+  int* x = new int();
+  delete x;
+}

test/lit.cfg

if config.clang_staticanalyzer:
     config.substitutions.append(
         ('%check_clang_tidy',
          '%s %s' % (config.python_executable, check_clang_tidy)) )
     clang_tidy_diff = os.path.join(
         config.test_source_root, "..", "clang-tidy", "tool", "clang-tidy-diff.py")
     config.substitutions.append(
         ('%clang_tidy_diff',
          '%s %s' % (config.python_executable, clang_tidy_diff)) )
+    run_clang_tidy = os.path.join(
+        config.test_source_root, "..", "clang-tidy", "tool", "run-clang-tidy.py")
+    config.substitutions.append(
+        ('%run_clang_tidy',
+         '%s %s' % (config.python_executable, run_clang_tidy)) )
 else:
     # exclude the clang-tidy test directory
     config.excludes.append('clang-tidy')