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

Please sort the imports

99

Please put continue on the next line.

105

This is going to be the case for each non-trivial invocation of this script. Do we need to keep MainSourceFile at all?

242

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

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

I'm not sure "fixfile" is a word. Just "file" maybe?

96

nit: "usid"

100

I'd use os.path.join() instead of concatenation.

104

nit: Add a trailing period.

109

nit: Add a trailing period.

[mfherbst]

Correct nits and typos.

[mfherbst]

run-clang-tidy.py

93

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!

run-clang-tidy.py

 
 from __future__ import print_function
 import argparse
+import glob
 import json
 import multiprocessing
 import os

 import tempfile
 import threading
 import traceback
+import yaml
 
 
 def find_compilation_database(path):

   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:

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

                       '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=['.*'],

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

     # 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()