[clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py

[LegalizeAdulthood]

Summary

Break up the huge function by extracting a class, storing intermediate
state as class members and breaking up the big function into a group
of class methods all at the same level of abstraction.

[LegalizeAdulthood]

Extract Function try_run

[JonasToth]

test/clang-tidy/check_clang_tidy.py

112

Its better to use .format() instead of % syntax

148–227

If would prefer keeping the if check_notes outside of the function call and remove that one argument. Same for the other check_... functions

[LegalizeAdulthood]

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

[JonasToth]

In D56343#1347352, @LegalizeAdulthood wrote:

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

I will give my best to be available this weekend but the review takes time until the patch is in an acceptable state, same for the other revision.
This process is required to keep good quality and catch bugs, even subtile ones early. Breaking code is the worst of all options.

[serge-sans-paille]

test/clang-tidy/check_clang_tidy.py

103

Such a long list of returned values make me wonder if it would be worth creating a NamedTuple / class to hold them.
This may make the call site more readable too.

141

These three check_* function all use the same 'FileCheck', '-....'] pattern. Maybe it's worth syndicating that to a try_run_filecheck(input_file0, input_file1, *extra_args)` function.

178

This is the verbose call site I was pointing to above.

[LegalizeAdulthood]

To avoid passing lots of state to/from extracted functions, extract a class instead and use member variables for most of the state

[JonasToth]

From my side its LGTM, but I would let @serge-sans-paille accept, as he is probably more familiar with python then I am.

[LegalizeAdulthood]

test/clang-tidy/check_clang_tidy.py

112

That can be done in a later commit; it is not the point of this review.

141

That can be done in a later commit; it is not the point of this review.

148–227

It's the wrong level of abstraction for the if check to be inside run()

178

Addressed by Extract Class instead of Extract Function

[serge-sans-paille]

Sorry for the long delay, this LGTM

[LegalizeAdulthood]

Rebase on top of tree

[LegalizeAdulthood]

This was accepted before in August, 2019, but the script changed in the meantime. I've repeated the Extract Class on top-of-tree,
so presumably this is still acceptable. However, I don't know how commit access has changed. (I don't think I have it anymore.)

[LegalizeAdulthood]

If there are no new comments by the end of the week, I'm going to accept this revision and commit it.

[aaron.ballman]

Resigning only because I don't think I have strong enough Python skills to give a quality review. I didn't spot anything that concerned me specific to this patch though.

test/clang-tidy/check_clang_tidy.py

 def write_file(file_name, text):
   with open(file_name, 'w') as f:
     f.write(text)
     f.truncate()
 
 def csv(string):
   return string.split(',')
 
-def main():
-  parser = argparse.ArgumentParser()
-  parser.add_argument('-expect-clang-tidy-error', action='store_true')
-  parser.add_argument('-resource-dir')
-  parser.add_argument('-assume-filename')
-  parser.add_argument('input_file_name')
-  parser.add_argument('check_name')
-  parser.add_argument('temp_file_name')
-  parser.add_argument('-check-suffix', '-check-suffixes',
-                      default=[''], type=csv,
-                      help="comma-separated list of FileCheck suffixes")
-
-  args, extra_args = parser.parse_known_args()
-
-  resource_dir = args.resource_dir
-  assume_file_name = args.assume_filename
-  input_file_name = args.input_file_name
-  check_name = args.check_name
-  temp_file_name = args.temp_file_name
-  expect_clang_tidy_error = args.expect_clang_tidy_error
-
-  file_name_with_extension = assume_file_name or input_file_name
-  _, extension = os.path.splitext(file_name_with_extension)
-  if extension not in ['.c', '.hpp', '.m', '.mm']:
-    extension = '.cpp'
-  temp_file_name = temp_file_name + extension
-
+def get_extra_args(resource_dir, extension, extra_args):
   clang_tidy_extra_args = extra_args
   if len(clang_tidy_extra_args) == 0:
     clang_tidy_extra_args = ['--']
     if extension in ['.cpp', '.hpp', '.mm']:
       clang_tidy_extra_args.append('--std=c++11')
     if extension in ['.m', '.mm']:
       clang_tidy_extra_args.extend(
           ['-fobjc-abi-version=2', '-fobjc-arc'])
 
   # Tests should not rely on STL being available, and instead provide mock
   # implementations of relevant APIs.
   clang_tidy_extra_args.append('-nostdinc++')
 
   if resource_dir is not None:
     clang_tidy_extra_args.append('-resource-dir=%s' % resource_dir)
 
-  with open(input_file_name, 'r') as input_file:
-    input_text = input_file.read()
+  return clang_tidy_extra_args
 
+def get_prefixes(check_suffix, input_text):
   check_fixes_prefixes = []
   check_messages_prefixes = []
   check_notes_prefixes = []
 
   has_check_fixes = False
   has_check_messages = False
   has_check_notes = False
 
-  for check in args.check_suffix:
+  for check in check_suffix:
     if check and not re.match('^[A-Z0-9\-]+$', check):
       sys.exit('Only A..Z, 0..9 and "-" are ' +
         'allowed in check suffixes list, but "%s" was given' % (check))
 
     file_check_suffix = ('-' + check) if check else ''
     check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
     check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
     check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
for check in check_suffix:
     has_check_fixes = has_check_fixes or has_check_fix
     has_check_messages = has_check_messages or has_check_message
     has_check_notes = has_check_notes or has_check_note
 
     check_fixes_prefixes.append(check_fixes_prefix)
     check_messages_prefixes.append(check_messages_prefix)
     check_notes_prefixes.append(check_notes_prefix)
 
-  assert has_check_fixes or has_check_messages or has_check_notes
-  # Remove the contents of the CHECK lines to avoid CHECKs matching on
-  # themselves.  We need to keep the comments to preserve line numbers while
-  # avoiding empty lines which could potentially trigger formatting-related
-  # checks.
-  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
-
-  write_file(temp_file_name, cleaned_test)
-
-  original_file_name = temp_file_name + ".orig"
-  write_file(original_file_name, cleaned_test)
-
-  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
-        clang_tidy_extra_args
-  if expect_clang_tidy_error:
-    args.insert(0, 'not')
-  print('Running ' + repr(args) + '...')
-  try:
-    clang_tidy_output = \
-        subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
-  except subprocess.CalledProcessError as e:
-    print('clang-tidy failed:\n' + e.output.decode())
-    raise
-
-  print('------------------------ clang-tidy output -----------------------\n' +
-        clang_tidy_output +
-        '\n------------------------------------------------------------------')
-
-  try:
-    diff_output = subprocess.check_output(
-        ['diff', '-u', original_file_name, temp_file_name],
-        stderr=subprocess.STDOUT)
-  except subprocess.CalledProcessError as e:
-    diff_output = e.output
-
-  print('------------------------------ Fixes -----------------------------\n' +
-        diff_output.decode() +
-        '\n------------------------------------------------------------------')
+  return (has_check_fixes, has_check_messages, has_check_notes, \
+    check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes)
 
+def check_fixes(check_fixes_prefixes, has_check_fixes, input_file_name, temp_file_name):
   if has_check_fixes:
     try:
       subprocess.check_output(
           ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
             '-check-prefixes=' + ','.join(check_fixes_prefixes),
             '-strict-whitespace'],
           stderr=subprocess.STDOUT)
     except subprocess.CalledProcessError as e:
       print('FileCheck failed:\n' + e.output.decode())
       raise
+  return
 
+def check_messages(check_messages_prefixes, has_check_messages, clang_tidy_output, input_file_name, temp_file_name):
   if has_check_messages:
     messages_file = temp_file_name + '.msg'
     write_file(messages_file, clang_tidy_output)
     try:
       subprocess.check_output(
           ['FileCheck', '-input-file=' + messages_file, input_file_name,
            '-check-prefixes=' + ','.join(check_messages_prefixes),
            '-implicit-check-not={{warning|error}}:'],
           stderr=subprocess.STDOUT)
     except subprocess.CalledProcessError as e:
       print('FileCheck failed:\n' + e.output.decode())
       raise
+  return
 
+def check_notes(check_notes_prefixes, has_check_notes, clang_tidy_output, input_file_name, temp_file_name):
   if has_check_notes:
     notes_file = temp_file_name + '.notes'
     filtered_output = [line for line in clang_tidy_output.splitlines()
                        if not "note: FIX-IT applied" in line]
     write_file(notes_file, '\n'.join(filtered_output))
     try:
       subprocess.check_output(
           ['FileCheck', '-input-file=' + notes_file, input_file_name,
            '-check-prefixes=' + ','.join(check_notes_prefixes),
            '-implicit-check-not={{note|warning|error}}:'],
           stderr=subprocess.STDOUT)
     except subprocess.CalledProcessError as e:
       print('FileCheck failed:\n' + e.output.decode())
       raise
+  return
+
+def main():
+  parser = argparse.ArgumentParser()
+  parser.add_argument('-expect-clang-tidy-error', action='store_true')
+  parser.add_argument('-resource-dir')
+  parser.add_argument('-assume-filename')
+  parser.add_argument('input_file_name')
+  parser.add_argument('check_name')
+  parser.add_argument('temp_file_name')
+  parser.add_argument('-check-suffix', '-check-suffixes',
+                      default=[''], type=csv,
+                      help="comma-separated list of FileCheck suffixes")
+
+  args, extra_args = parser.parse_known_args()
+
+  resource_dir = args.resource_dir
+  assume_file_name = args.assume_filename
+  input_file_name = args.input_file_name
+  check_name = args.check_name
+  temp_file_name = args.temp_file_name
+  expect_clang_tidy_error = args.expect_clang_tidy_error
+
+  file_name_with_extension = assume_file_name or input_file_name
+  _, extension = os.path.splitext(file_name_with_extension)
+  if extension not in ['.c', '.hpp', '.m', '.mm']:
+    extension = '.cpp'
+  temp_file_name = temp_file_name + extension
+
+  clang_tidy_extra_args = get_extra_args(resource_dir, extension, extra_args)
+
+  with open(input_file_name, 'r') as input_file:
+    input_text = input_file.read()
+
+  has_check_fixes, has_check_messages, has_check_notes, \
+    check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes = \
+    get_prefixes(args.check_suffix, input_text)
+
+  assert has_check_fixes or has_check_messages or has_check_notes
+
+  # Remove the contents of the CHECK lines to avoid CHECKs matching on
+  # themselves.  We need to keep the comments to preserve line numbers while
+  # avoiding empty lines which could potentially trigger formatting-related
+  # checks.
+  cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
+  write_file(temp_file_name, cleaned_test)
+  original_file_name = temp_file_name + ".orig"
+  write_file(original_file_name, cleaned_test)
+
+  args = ['clang-tidy', temp_file_name, '-fix', '--checks=-*,' + check_name] + \
+        clang_tidy_extra_args
+  if expect_clang_tidy_error:
+    args.insert(0, 'not')
+  print('Running ' + repr(args) + '...')
+  try:
+    clang_tidy_output = \
+        subprocess.check_output(args, stderr=subprocess.STDOUT).decode()
+  except subprocess.CalledProcessError as e:
+    print('clang-tidy failed:\n' + e.output.decode())
+    raise
+
+  print('------------------------ clang-tidy output -----------------------\n' +
+        clang_tidy_output +
+        '\n------------------------------------------------------------------')
+
+  try:
+    diff_output = subprocess.check_output(
+        ['diff', '-u', original_file_name, temp_file_name],
+        stderr=subprocess.STDOUT)
+  except subprocess.CalledProcessError as e:
+    diff_output = e.output
+
+  print('------------------------------ Fixes -----------------------------\n' +
+        diff_output.decode() +
+        '\n------------------------------------------------------------------')
+
+  check_fixes(check_fixes_prefixes, has_check_fixes, input_file_name, temp_file_name)
+  check_messages(check_messages_prefixes, has_check_messages, clang_tidy_output, input_file_name, temp_file_name)
+  check_notes(check_notes_prefixes, has_check_notes, clang_tidy_output, input_file_name, temp_file_name)
 
 if __name__ == '__main__':
   main()