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

[mfherbst]

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.

[mfherbst]

Fixed typos (MainFile => MainSourceFile)

[alexfh]

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.

[mfherbst]

• Adapted patch to changes suggested by alexfh
• Added comment why setting MainSourceFile to an empty string if fixes are exported

[mfherbst]

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]

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.

[mfherbst]

Correct nits and typos.

[mfherbst]

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]

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.

[mfherbst]

Correct the nits suggested by alexfh

[mfherbst]

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.

[alexfh]

In D31326#803535, @mfherbst wrote:

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]

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]

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.

[mfherbst]

Remove compatiblity with clang-tidy < 4.0.0

[alexfh]

LG. I'll commit the patch for you

[mfherbst]

Many thanks!

[alexfh]

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!

clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py

run-clang-tidy.py -fix -checks=-*,llvm-header-guard extra/clang-tidy \
                       -header-filter=extra/clang-tidy
 
 Compilation database setup:
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
 from __future__ import print_function
 import argparse
+import glob
 import json
 import multiprocessing
 import os
 import Queue
 import re
 import shutil
 import subprocess
 import sys
 import tempfile
 import threading
 import traceback
+import yaml
 
 
 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.')
for arg in extra_arg_before:
       start.append('-extra-arg-before=%s' % arg)
   start.append('-p=' + build_path)
   if quiet:
       start.append('-quiet')
   start.append(f)
   return start
 
 
+def merge_replacement_files(tmpdir, mergefile):
+  """Merge all replacement files in a directory into a single file"""
+  # The fixes suggested by clang-tidy >= 4.0.0 are given under
+  # the top level key 'Diagnostics' in the output yaml files
+  mergekey="Diagnostics"
+  merged=[]
+  for replacefile in glob.iglob(os.path.join(tmpdir, '*.yaml')):
+    content = yaml.safe_load(open(replacefile, 'r'))
+    if not content:
+      continue # Skip empty files.
+    merged.extend(content.get(mergekey, []))
+
+  if merged:
+    # MainSourceFile: The key is required by the definition inside
+    # include/clang/Tooling/ReplacementsYaml.h, but the value
+    # is actually never used inside clang-apply-replacements,
+    # so we set it to '' here.
+    output = { 'MainSourceFile': '', mergekey: merged }
+    with open(mergefile, 'w') as out:
+      yaml.safe_dump(output, out)
+  else:
+    # Empty the file:
+    open(mergefile, 'w').close()
+
+
 def check_clang_apply_replacements_binary(args):
   """Checks if invoking supplied clang-apply-replacements binary works."""
   try:
     subprocess.check_call([args.clang_apply_replacements_binary, '--version'])
   except:
     print('Unable to run clang-apply-replacements. Is clang-apply-replacements '
           'binary correctly specified?', file=sys.stderr)
     traceback.print_exc()
     sys.exit(1)
 
 
 def apply_fixes(args, tmpdir):
-  """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
+  """Calls clang-apply-fixes on a given directory."""
   invocation = [args.clang_apply_replacements_binary]
   if args.format:
     invocation.append('-format')
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
   subprocess.call(invocation)
 
def main():
   parser.add_argument('-checks', default=None,
                       help='checks filter, when not specified, use clang-tidy '
                       'default')
   parser.add_argument('-header-filter', default=None,
                       help='regular expression matching the names of the '
                       'headers to output diagnostics from. Diagnostics from '
                       'the main file of each translation unit are always '
                       'displayed.')
+  parser.add_argument('-export-fixes', metavar='filename', dest='export_fixes',
+                      help='Create a yaml file to store suggested fixes in, '
+                      'which can be applied with clang-apply-replacements.')
   parser.add_argument('-j', type=int, default=0,
                       help='number of tidy instances to be run in parallel.')
   parser.add_argument('files', nargs='*', default=['.*'],
                       help='files to be processed (regex on path)')
   parser.add_argument('-fix', action='store_true', help='apply fix-its')
   parser.add_argument('-format', action='store_true', help='Reformat code '
                       'after applying fixes')
   parser.add_argument('-style', default='file', help='The style of reformat '
def main():
   database = json.load(open(os.path.join(build_path, db_path)))
   files = [entry['file'] for entry in database]
 
   max_task = args.j
   if max_task == 0:
     max_task = multiprocessing.cpu_count()
 
   tmpdir = None
-  if args.fix:
+  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))
 
   try:
     # Spin up a bunch of tidy-launching threads.
try:
 
     # Wait for all threads to be done.
     queue.join()
 
   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 args.fix:
+    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
+
   if args.fix:
     print('Applying fixes ...')
-    successfully_applied = False
-
     try:
       apply_fixes(args, tmpdir)
-      successfully_applied = True
     except:
       print('Error applying fixes.\n', file=sys.stderr)
       traceback.print_exc()
+      return_code=1
 
+  if tmpdir:
     shutil.rmtree(tmpdir)
-    if not successfully_applied:
-      sys.exit(1)
+  sys.exit(return_code)
 
 if __name__ == '__main__':
   main()