[PATCH] Enable clang-tidy misc-static-assert for C11

[aaron.ballman]

Summary

This patch adds support for the C11 _Static_assert macro to clang-tidy's misc-static-assert checker. There are a few things I'm not overly satisfied with, and welcome suggestions on how to improve, if desired.

1. If <assert.h> is included, then static_assert() is also fine to use. Presumably, in order for the assert() check to trigger, <assert.h> has to be included, but I don't feel particularly comfortable with that assumption. If there's a way to access a Preprocessor instance from check(), we can use getMacroDefinitionAtLoc() to determine whether static_assert() is defined and a valid replacement for assert(). For right now, I'm always using _Static_assert() in C11 mode to be on the safe side.
2. My Python skills are rudimentary at best. If there's a better way to update check_clang_tidy.py to handle file extensions, that would be great. This change is required or else the script fails on .c test cases that check fixes because of writing out to a .tmp.cpp file instead of a .tmp.c file conflicts with the language options specified on the RUN line.

~Aaron

[alexfh]

1. If <assert.h> is included, then static_assert() is also fine to use. Presumably, in order for the assert() check to trigger, <assert.h> has to be

included, but I don't feel particularly comfortable with that assumption.

Do you have any C11 code that uses assert(), but doesn't include assert.h? If no, I'd suggest relying on this assumption until someone stumbles upon this and files a bug (or fixes this). One more option is to ensure assert.h is always included in the file being fixed (there's IncludeInserter class that can do this).

If there's a way to access a Preprocessor instance from check(),

You can subscribe to PPCallbacks and find out if static_assert is defined after the last include. Or if it's defined and not undefined. But I'd probably just skip this until you see real code that would benefit from this.

we can use getMacroDefinitionAtLoc() to determine whether static_assert() is defined and a valid replacement for assert(). For right now, I'm
always using _Static_assert() in C11 mode to be on the safe side.

2. My Python skills are rudimentary at best. If there's a better way to update check_clang_tidy.py to handle file extensions, that would be great.

This change is required or else the script fails on .c test cases that check fixes because of writing out to a .tmp.cpp file instead of a .tmp.c file
conflicts with the language options specified on the RUN line.

Alternatively, you could add "-x c" to the test's RUN: line, but your solution is an improvement over this hack. Thanks!

test/clang-tidy/check_clang_tidy.py

50

I'm not sure that c99 is a good default for C code here. The existing .c tests don't rely on this, and the tests you're adding override -std anyway. I'd change this to ['--', '--std=c++11'] if extension == '.cpp' else ['--'].

test/clang-tidy/misc-static-assert-c99.c

1 ↗

(On Diff #33461)

Instead of duplicating the test, you could add a second run line in the other test and just verify that clang-tidy doesn't generate any warnings when run with -std=c99. The invariant "no warnings == no fixes" is pretty basic for clang-tidy, so you can rely on it in the tests.

[aaron.ballman]

No longer worrying about static_assert() vs _Static_assert(). Corrected Python script.

[aaron.ballman]

test/clang-tidy/misc-static-assert-c99.c

2 ↗

(On Diff #33579)

I am not certain how to accomplish this with the python script. Or are you thinking I should run clang-tidy directly and use "| count 0" ?

[aaron.ballman]

It seems Phab doesn't give you any context for this comment. For
reference, this is in response to the comment:

Instead of duplicating the test, you could add a second run line in
the other test and just verify that clang-tidy doesn't generate any
warnings when run with -std=c99. The invariant "no warnings == no
fixes" is pretty basic for clang-tidy, so you can rely on it in the
tests.

~Aaron

[alexfh]

test/clang-tidy/misc-static-assert-c99.c

2 ↗

(On Diff #33579)

I was thinking about testing c11 mode using the script and c99 mode can run clang-tidy directly (without -fix) and use | count 0, | grep -v 'warning:' or anything else with the same meaning.

[klimek]

test/clang-tidy/misc-static-assert-c99.c

2 ↗

(On Diff #33579)

Phab test.

1 ↗

(On Diff #33461)

Phab test 2.

[aaron.ballman]

test/clang-tidy/misc-static-assert-c99.c

2 ↗

(On Diff #33579)

Phab test.

[aaron.ballman]

Only use a single test file with two RUN lines.

[alexfh]

LG

[aaron.ballman]

Thanks! I've commit in r246494 and r246495.

clang-tidy/misc/StaticAssertCheck.cpp

     : ClangTidyCheck(Name, Context) {}
 
 void StaticAssertCheck::registerMatchers(MatchFinder *Finder) {
-  // FIXME: I don't see why this checker couldn't also be interesting for
-  // _Static_assert in C11, or static_assert if <assert.h> has been included, 
-  // but it is currently only enabled for C++11. Investigate.
-  if (!getLangOpts().CPlusPlus11)
+  // This checker only makes sense for languages that have static assertion
+  // capabilities: C++11 and C11.
+  if (!(getLangOpts().CPlusPlus11 || getLangOpts().C11))
     return;
 
   auto IsAlwaysFalse = expr(ignoringParenImpCasts(

test/clang-tidy/check_clang_tidy.py

     sys.exit('Not enough arguments.')
 
   input_file_name = sys.argv[1]
+  extension = '.cpp'
+  if (input_file_name.endswith('.c')):
+    extension = '.c'
+    
   check_name = sys.argv[2]
-  temp_file_name = sys.argv[3] + '.cpp'
+  temp_file_name = sys.argv[3] + extension
 
   clang_tidy_extra_args = sys.argv[4:]
   if len(clang_tidy_extra_args) == 0:
-    clang_tidy_extra_args = ['--', '--std=c++11']
+    clang_tidy_extra_args = ['--', '--std=c++11'] if extension == '.cpp' else ['--']
 
   with open(input_file_name, 'r') as input_file:
     input_text = input_file.read()

test/clang-tidy/misc-static-assert.c

+// RUN: %python %S/check_clang_tidy.py %s misc-static-assert %t -- -std=c11
+// RUN: clang-tidy %s -checks=-*,misc-static-assert -- -std=c99 | count 0
+
+void abort() {}
+#ifdef NDEBUG
+#define assert(x) 1
+#else
+#define assert(x)                                                              \
+  if (!(x))                                                                    \
+  abort()
+#endif
+
+void f(void) {
+  int x = 1;
+  assert(x == 0);
+  // CHECK-FIXES: {{^  }}assert(x == 0);
+
+  #define static_assert(x, msg) _Static_assert(x, msg)
+  assert(11 == 5 + 6);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be
+  // CHECK-FIXES: {{^  }}static_assert(11 == 5 + 6, "");
+  #undef static_assert
+
+  assert(10 == 5 + 5);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: found assert() that could be
+  // CHECK-FIXES: {{^  }}static_assert(10 == 5 + 5, "");
+}