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

[zinovy.nis]

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

[zinovy.nis]

Any comments?

[alexfh]

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

[zinovy.nis]

In D59449#1435032, @alexfh wrote:

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.

[thakis]

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

[zinovy.nis]

In D59449#1446839, @thakis wrote:

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.

[zinovy.nis]

Added a test.

[zinovy.nis]

Gentle ping.

[alexfh]

In D59449#1435836, @zinovy.nis wrote:

In D59449#1435032, @alexfh wrote:

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?

[zinovy.nis]

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.

[zinovy.nis]

@alexfh, do you still have any objections?

[alexfh]

In D59449#1449664, @zinovy.nis wrote:

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)

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

 #!/usr/bin/env python
 #
 #===- run-clang-tidy.py - Parallel clang-tidy runner ---------*- python -*--===#
 #
 # Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 # See https://llvm.org/LICENSE.txt for license information.
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===------------------------------------------------------------------------===#
-# FIXME: Integrate with clang-tidy-diff.py
 
 """
 Parallel clang-tidy runner
 ==========================
 
 Runs clang-tidy over all files in a compilation database. Requires clang-tidy
 and clang-apply-replacements in $PATH.
 

 
 is_py2 = sys.version[0] == '2'
 
 if is_py2:
     import Queue as queue
 else:
     import queue as queue
 
+
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
     if os.path.realpath(result) == '/':
       print('Error: could not find compilation database.')
       sys.exit(1)
     result += '../'
   return os.path.realpath(result)
 
 
 def make_absolute(f, directory):
   if os.path.isabs(f):
     return f
   return os.path.normpath(os.path.join(directory, f))
 
 
-def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
+def get_tidy_invocation(f, lines, clang_tidy_binary, checks, tmpdir, build_path,
                         header_filter, extra_arg, extra_arg_before, quiet,
                         config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
   if header_filter is not None:
     start.append('-header-filter=' + header_filter)
   else:
     # Show warnings in all in-project headers by default.
     start.append('-header-filter=^' + build_path + '/.*')
+
+  if lines is not None:
+    line_json = json.dumps([{"name": f, "lines": lines}], separators=(',', ':'))
+    # Run clang-tidy on files containing changes.
+    start.append('-line-filter=' + line_json)
+
   if checks:
     start.append('-checks=' + checks)
   if tmpdir is not None:
     start.append('-export-fixes')
     # Get a temporary file. We immediately close the handle so clang-tidy can
     # overwrite it.
     (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir)
     os.close(handle)
if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
 
 def run_tidy(args, tmpdir, build_path, queue, lock, 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,
+    (name, lines) = queue.get()
+    invocation = get_tidy_invocation(name, lines,
+                                     args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
                                      args.extra_arg, args.extra_arg_before,
                                      args.quiet, args.config)
 
     proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     output, err = proc.communicate()
     if proc.returncode != 0:
       failed_files.append(name)
     with lock:
       sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
       if len(err) > 0:
         sys.stderr.write(err.decode('utf-8') + '\n')
     queue.task_done()
 
 
+def get_changed_lines(prefix_len):
+  iregex = '^%s$' % r'.*\.(cpp|cc|c\+\+|cxx|c|cl|h|hpp|m|mm|inc)'
+  lines_by_file = {}
+  filename = None
+  for line in sys.stdin:
+    match = re.search('^\+\+\+\ \"?(.*?/){%s}([^ \t\n\"]*)' % prefix_len, line)
+    if match:
+      filename = match.group(2)
+    if filename is None:
+      continue
+
+    if not re.match(iregex, filename, re.IGNORECASE):
+      continue
+
+    match = re.search('^@@.*\+(\d+)(,(\d+))?', line)
+    if match:
+      start_line = int(match.group(1))
+      line_count = 1
+      if match.group(3):
+        line_count = int(match.group(3))
+      if line_count == 0:
+        continue
+      end_line = start_line + line_count - 1
+      lines_by_file.setdefault(filename, []).append([start_line, end_line])
+
+  return lines_by_file
+
+
 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.')
+  parser.add_argument('--diff', default=None, const=2, nargs='?', type=int,
+                      help='check only the diff read from stdin.'
+                      ' Strip the smallest prefix containing DIFF[=2] slashes')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
                       default='clang-tidy',
                       help='path to clang-tidy binary')
   parser.add_argument('-clang-apply-replacements-binary', metavar='PATH',
                       default='clang-apply-replacements',
                       help='path to clang-apply-replacements binary')
   parser.add_argument('-checks', default=None,
                       help='checks filter, when not specified, use clang-tidy '
try:
     if args.checks:
       invocation.append('-checks=' + args.checks)
     invocation.append('-')
     subprocess.check_call(invocation)
   except:
     print("Unable to run clang-tidy.", file=sys.stderr)
     sys.exit(1)
 
-  # Load the database and extract all files.
-  database = json.load(open(os.path.join(build_path, db_path)))
-  files = [make_absolute(entry['file'], entry['directory'])
-           for entry in database]
-
   max_task = args.j
   if max_task == 0:
     max_task = multiprocessing.cpu_count()
 
   tmpdir = None
   if args.fix or (yaml and 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))
+  files = None
+  changed_lines = None
+  if args.diff is not None:
+    changed_lines = get_changed_lines(args.diff)
+    files = [(k, v) for k,v in changed_lines.items() if file_name_re.search(k)]
+  else:
+    # Load the database and extract affected files.
+    database = json.load(open(os.path.join(build_path, db_path)))
+    all_files = (make_absolute(entry['file'], entry['directory']) for entry in database)
+    files = [(x, None) for x in all_files if file_name_re.search(x)]
 
   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 = []
     lock = threading.Lock()
     for _ in range(max_task):
       t = threading.Thread(target=run_tidy,
                            args=(args, tmpdir, build_path, task_queue, lock, 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

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

+// REQUIRES: shell
+// RUN: mkdir -p "%t"
+// RUN: cd       "%t"
+// RUN: sed 's/placeholder_for_f/f/' %s > diff_to.cpp
+// RUN: echo '[{"directory": "%t", "command": "clang++ -o test.o -std=c++11 diff_to.cpp", "file": "diff_to.cpp"}]' > compile_commands.json
+// RUN: clang-tidy -checks=-*,modernize-use-override diff_to.cpp -- -std=c++11 | FileCheck -check-prefix=CHECK-SANITY %s
+// RUN: not diff -U0 %s diff_to.cpp | %run_clang_tidy --diff=0 -checks=-*,modernize-use-override        2>&1 | FileCheck %s
+// RUN: not diff -U0 %s diff_to.cpp | %run_clang_tidy --diff=0 -checks=-*,modernize-use-override -quiet 2>&1 | FileCheck -check-prefix=CHECK-QUIET %s
+// RUN: not diff -U0 %s diff_to.cpp | %run_clang_tidy --diff=0 -checks=-*,modernize-use-override        2>&1 | FileCheck -check-prefix=CHECK %s
+struct A {
+  virtual void f() {}
+  virtual void g() {}
+};
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+struct B : public A {
+  void placeholder_for_f() {}
+// CHECK-SANITY: [[@LINE-1]]:8: warning: annotate this
+// CHECK: [[@LINE-2]]:8: warning: annotate this
+// CHECK-QUIET: [[@LINE-3]]:8: warning: annotate this
+  void g() {}
+// CHECK-SANITY: [[@LINE-1]]:8: warning: annotate this
+// CHECK-NOT: warning:
+// CHECK-QUIET-NOT: warning:
+};
+// CHECK-SANITY-NOT: Suppressed
+// CHECK-QUIET-NOT: Suppressed