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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 9 inline comments as done.
LegalizeAdulthood added inline comments.


================
Comment at: test/clang-tidy/check_clang_tidy.py:112
+    process_output = e.output.decode()
+    print('%s failed:\n%s' % (' '.join(args), process_output))
+    if raise_error:
----------------
JonasToth wrote:
> Its better to use `.format()` instead of `%` syntax
That can be done in a later commit; it is not the point of this review.


================
Comment at: test/clang-tidy/check_clang_tidy.py:141
+           '-check-prefixes=' + ','.join(check_notes_prefixes),
+           '-implicit-check-not={{note|warning|error}}:'])
+  return
----------------
serge-sans-paille wrote:
> 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.
That can be done in a later commit; it is not the point of this review.


================
Comment at: test/clang-tidy/check_clang_tidy.py:178
+    check_fixes_prefixes, check_messages_prefixes, check_notes_prefixes = \
+    get_prefixes(args.check_suffix, input_text)
+
----------------
serge-sans-paille wrote:
> This is the verbose call site I was pointing to above.
Addressed by Extract Class instead of Extract Function


================
Comment at: test/clang-tidy/check_clang_tidy.py:206
 
-  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
-
-  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
-
-  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
+  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)
----------------
JonasToth wrote:
> If would prefer keeping the `if check_notes` outside of the function call and remove that one argument. Same for the other `check_...` functions
It's the wrong level of abstraction for the `if` check to be inside `run()`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56343/new/

https://reviews.llvm.org/D56343