[clang-tidy] Customize FileCheck prefix in check_clang-tidy.py to support multiple prefixes

[zinovy.nis]

Summary

The patch extends the existing command line option -check-suffix (with alias -check-suffixes) to accept multiple comma-separated prefixes.

// RUN: %check_clang_tidy -check-suffix=USING-C,USING-D %s misc-unused-using-decls %t -- -- ...

or for the same

// RUN: %check_clang_tidy -check-suffixes=USING-C,USING-D %s misc-unused-using-decls %t -- -- ...

[lebedev.ri]

Thank you!

test/clang-tidy/check_clang_tidy.py

94–95 ↗

(On Diff #168599)

How , can be allowed if that is the suffix separator?
Also, i think _ can be supported.

[zinovy.nis]

Removed "," from the list of allowed suffix characters.

[zinovy.nis]

test/clang-tidy/check_clang_tidy.py

94–95 ↗

(On Diff #168599)

When discussing the previous patch Alex said on underscores:

I don't know whether it makes sense to endorse (or even allow) the use of underscore in the check suffix. The mix of underscores and dashes looks ugly and is prone to errors.

So it was intentionally.

[JonasToth]

The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?

test/clang-tidy/check_clang_tidy.py

112 ↗

(On Diff #168609)

i think the whitespace in this line should be aligned with with the line above

[alexfh]

Looks good with a comment.

test/clang-tidy/check_clang_tidy.py

52 ↗

(On Diff #168609)

Maybe -check-suffixes to make it more obvious that multiple values are supported?

[zinovy.nis]

In D52971#1257702, @JonasToth wrote:

The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?

Yes, all the existing tests pass successfully.

[zinovy.nis]

In D52971#1257705, @alexfh wrote:

Looks good with a comment.

It can break the compatibility. May be it worth to add an alias "-check-suffixes" for this option?

[zinovy.nis]

• Fixed diagnostics;
• Interchangeable alias introduced: -check-suffixes for -check-suffix.

[zinovy.nis]

In D52971#1257702, @JonasToth wrote:

I think it would make sense to migrate one test already, to use the new capability

This change was inspired by @lebedev.ri, so we expect him to provide us with such a test soon ;-)

[lebedev.ri]

In D52971#1258086, @zinovy.nis wrote:

In D52971#1257702, @JonasToth wrote:

I think it would make sense to migrate one test already, to use the new capability

This change was inspired by @lebedev.ri, so we expect him to provide us with such a test soon ;-)

This will hopefully deduplicate some check lines in D52771 test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

[ymandel]

Hi,

It looks like this change has disabled FileCheck for all clang-tidy lit tests that don't use check-prefixes. So, they all trivially pass even if their CHECK... lines are wrong. An easy repro is just to randomly modify any CHECK line in a lit file (e.g. llvm/tools/clang/tools/extra/test/clang-tidy/readability-avoid-const-params-in-decls.cpp) and run ninja check-clang-tools.

In check_clang_tidy.py, if you add back (slightly modified) lines 93-95 to the else branch (line 132), it seems to fix the problem. For example, add:

has_check_fixes = check_fixes_prefixes[0] in input_text
has_check_messages = check_messages_prefixes[0] in input_text
has_check_notes = check_notes_prefixes[0] in input_text

[zinovy.nis]

In D52971#1262200, @ymandel wrote:

Hi,

It looks like this change has disabled FileCheck for all clang-tidy lit tests that don't use check-prefixes. So, they all trivially pass even if their CHECK... lines are wrong. An easy repro is just to randomly modify any CHECK line in a lit file (e.g. llvm/tools/clang/tools/extra/test/clang-tidy/readability-avoid-const-params-in-decls.cpp) and run ninja check-clang-tools.

In check_clang_tidy.py, if you add back (slightly modified) lines 93-95 to the else branch (line 132), it seems to fix the problem. For example, add:

has_check_fixes = check_fixes_prefixes[0] in input_text
has_check_messages = check_messages_prefixes[0] in input_text
has_check_notes = check_notes_prefixes[0] in input_text

Oh, thanks.
Here's a fix: https://reviews.llvm.org/D53194
Sorry for inconvenience.

clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy.cpp

 // RUN: %check_clang_tidy -check-suffix=USING-A %s misc-unused-using-decls %t -- -- -DUSING_A
 // RUN: %check_clang_tidy -check-suffix=USING-B %s misc-unused-using-decls %t -- -- -DUSING_B
+// RUN: %check_clang_tidy -check-suffix=USING-C,USING-D %s misc-unused-using-decls %t -- -- -DUSING_C_D
+// RUN: %check_clang_tidy -check-suffixes=USING-C,USING-D %s misc-unused-using-decls %t -- -- -DUSING_C_D
 // RUN: %check_clang_tidy %s misc-unused-using-decls %t
 
-namespace a {class A {}; class B {}; class C {}; }
+namespace a {class A {}; class B {}; class C {}; class D {}; class E {};}
 namespace b {
 #if defined(USING_A)
 using a::A;
 #elif  defined(USING_B)
 using a::B;
-#else
+#elif  defined(USING_C_D)
 using a::C;
+using a::D;
+#else
+using a::E;
 #endif
 }
 namespace c {}
-// CHECK-MESSAGES-USING-A: :[[@LINE-8]]:10: warning: using decl 'A' {{.*}}
-// CHECK-MESSAGES-USING-B: :[[@LINE-7]]:10: warning: using decl 'B' {{.*}}
-// CHECK-MESSAGES: :[[@LINE-6]]:10: warning: using decl 'C' {{.*}}
+// CHECK-MESSAGES-USING-A: warning: using decl 'A' {{.*}}
+// CHECK-MESSAGES-USING-B: warning: using decl 'B' {{.*}}
+// CHECK-MESSAGES-USING-C: warning: using decl 'C' {{.*}}
+// CHECK-MESSAGES-USING-D: warning: using decl 'D' {{.*}}
+// CHECK-MESSAGES: warning: using decl 'E' {{.*}}
 // CHECK-FIXES-USING-A-NOT: using a::A;$
 // CHECK-FIXES-USING-B-NOT: using a::B;$
-// CHECK-FIXES-NOT: using a::C;$
+// CHECK-FIXES-USING-C-NOT: using a::C;$
+// CHECK-FIXES-USING-C-NOT: using a::D;$
+// CHECK-FIXES-USING-D-NOT: using a::C;$
+// CHECK-FIXES-USING-D-NOT: using a::D;$
+// CHECK-FIXES-NOT: using a::E;$

clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy.py

 ClangTidy Test Helper
 =====================
 
 This script runs clang-tidy in fix mode and verify fixes, messages or both.
 
 Usage:
   check_clang_tidy.py [-resource-dir=<resource-dir>] \
     [-assume-filename=<file-with-source-extension>] \
-    [-check-suffix=<file-check-suffix>] \
+    [-check-suffix=<comma-separated-file-check-suffixes>] \
+    [-check-suffixes=<comma-separated-file-check-suffixes>] \
     <source-file> <check-name> <temp-file> \
     -- [optional clang-tidy arguments]
 
 Example:
   // RUN: %check_clang_tidy %s llvm-include-order %t -- -- -isystem %S/Inputs
 """
 
 import argparse
 import os
 import re
 import subprocess
 import sys
 
 
 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('-check-suffix', default='')
   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
def main():
   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'])
 
-  if args.check_suffix and not re.match('^[A-Z0-9\-]+$', args.check_suffix):
-    sys.exit('Only A..Z, 0..9 and "-" are allowed in check suffix, but "%s" was given' % (args.check_suffix))
-
-  file_check_suffix = ('-' + args.check_suffix) if args.check_suffix 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
-
   # 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()
 
-  has_check_fixes = check_fixes_prefix in input_text
-  has_check_messages = check_messages_prefix in input_text
-  has_check_notes = check_notes_prefix in input_text
-
-  if not has_check_fixes and not has_check_messages and not has_check_notes:
-    sys.exit('%s, %s or %s not found in the input' % (check_fixes_prefix,
-             check_messages_prefix, check_notes_prefix) )
+  check_fixes_prefixes = []
+  check_messages_prefixes = []
+  check_notes_prefixes = []
+
+  has_check_fixes = False
+  has_check_messages = False
+  has_check_notes = False
+
+  if any(args.check_suffix):
+    for check in args.check_suffix:
+      if 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
+      check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
+      check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
+      check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
 
-  if has_check_notes and has_check_messages:
-    sys.exit('Please use either CHECK-NOTES or CHECK-MESSAGES but not both')
+      has_check_fix = check_fixes_prefix in input_text 
+      has_check_message = check_messages_prefix in input_text
+      has_check_note = check_notes_prefix in input_text 
+
+      if has_check_note and has_check_message:
+        sys.exit('Please use either %s or %s but not both' %
+          (check_notes_prefix, check_messages_prefix))
+
+      if not has_check_fix and not has_check_message and not has_check_note:
+        sys.exit('%s, %s or %s not found in the input' %
+          (check_fixes_prefix, check_messages_prefix, check_notes_prefix))
+
+      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)
+  else:
+    check_fixes_prefixes = ['CHECK-FIXES']
+    check_messages_prefixes = ['CHECK-MESSAGES']
+    check_notes_prefixes = ['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)
def main():
   print('------------------------------ Fixes -----------------------------\n' +
         diff_output.decode() +
         '\n------------------------------------------------------------------')
 
   if has_check_fixes:
     try:
       subprocess.check_output(
           ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
-           '-check-prefix=' + check_fixes_prefix, '-strict-whitespace'],
+           '-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-prefix=' + check_messages_prefix,
+           '-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-prefix=' + check_notes_prefix,
+           '-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
 
 if __name__ == '__main__':
   main()