[PATCH] D15710: [clang-tidy] Add non-inline function definition and variable definition check in header files.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:52
@@ +51,3 @@
+
+  // Internal linkage variable and function definitions are allowed:
+  //   const int a = 1;
----------------
I'd say "ignored for now" instead of "allowed". We might want to flag them in the future, but these are used quite frequently, so we don't warn on these now to keep the number of warnings under control.

Maybe there should be an option for these cases.

================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:8
@@ +7,3 @@
+   // Foo.h
+   int a = 1; // error
+   static int b = 1; // ok
----------------
nit: It's a warning, not error. Also, please use proper capitalization and punctuation:

```
int a = 1; // Warning.
```

or maybe even include the message:

```
int a = 1; // Warning: "variable definition is not allowed in header file".
```

Same holds for `// ok`: They should either be `// OK.` or maybe explain why certain patterns don't generate a warning.

================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:10
@@ +9,3 @@
+class CA {
+  void f1() {} // ok
+  void f2();
----------------
The `// ok` comments add no new information. Either explain why are these OK, or just remove the comments.

================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:27
@@ +26,3 @@
+void CA::f2() { }
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: function definition is not allowed in header file [misc-definitions-in-headers]
+// CHECK-FIXES: inline void CA::f2() {
----------------
We usually leave each distinct warning message full once and truncate the other CHECK-MESSAGES patterns to fit to the 80 characters limit (e.g. here I'd remove the `in header file [misc-definitions-in-headers]` part).

================
Comment at: test/lit.cfg:46
@@ -45,2 +45,3 @@
 # suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.c', '.cpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s', '.modularize', '.module-map-checker']
+config.suffixes = ['.c', '.cpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s',
+  '.modularize', '.module-map-checker', '.hpp']
----------------
Please place the new extension after '.cpp' - it seems to be most relevant there.


http://reviews.llvm.org/D15710