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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Looks good with a few nits. Please fix them and I'll submit the patch for you.

Thank you for working on this!


================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:23
@@ +22,3 @@
+AST_MATCHER(NamedDecl, isHeaderFileExtension) {
+  SourceManager& SM = Finder->getASTContext().getSourceManager();
+  SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
----------------
Actually, this name is also confusing, but I don't have a better idea now. Let's leave it like this.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:25
@@ +24,3 @@
+  SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart());
+  if (SM.isInSystemHeader(ExpansionLoc))
+    return false;
----------------
I think, this should be opposite for two reasons:
1. variable definitions and declarations in system headers are not better than in user headers and can lead to the same problems;
2. there are people who care about system headers, so we need to produce diagnostics on them and let clang-tidy filter them out when not needed.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.h:23
@@ +22,3 @@
+// There is one option:
+// - `UseHeaderFileExtension`: Whether to use file extension(h, hh, hpp, hxx) to
+//   distinguish header files. True by default.
----------------
nit: add a space before the '('

================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:15
@@ +14,3 @@
+
+   // Internal linkage variable definitions are ignored for now,
+   // Although these might also cause ODR violations, we can be less certain and
----------------
nit: the sentence should end with a period, not a comma.

================
Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:4
@@ +3,3 @@
+int f() {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file;
+// CHECK-FIXES: inline int f() {
----------------
Please specify each distinct warning message (in your case, there are two of them) completely (including the check name) once. Having it in the test (just once) is useful to verify that the message is formatted correctly (in your case, there's no space after the ';', for example).


http://reviews.llvm.org/D15710