[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 almost good to me. A few more comments though.


================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:22
@@ +21,3 @@
+
+AST_MATCHER(NamedDecl, useHeaderFileExtension) {
+  SourceManager& SM = Finder->getASTContext().getSourceManager();
----------------
nit: This name is not clear to me. Did you mean `isHeaderFileExtension` or `usesHeaderFileExtension`?

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:28
@@ +27,3 @@
+         Filename.endswith(".hpp") || Filename.endswith(".hxx") ||
+         (!SM.isInSystemHeader(ExpansionLoc) && Filename.endswith(""));
+}
----------------
I'm going to disappoint you: any string ends with an empty string ;)

You'll need a better logic for comparing extensions. `llvm::sys::path::extension()` might be useful here.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:80
@@ +79,3 @@
+  if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
+    // Inline function is allowed.
+    if (FD->isInlined())
----------------
nit: I'd say in plural: `Inline functions are allowed.`. Same below.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:115
@@ +114,3 @@
+    diag(VD->getLocation(),
+         "variable definition is not allowed in header file");
+  }
----------------
Maybe add the name of the variable to the message? This would help users to understand the cause of the warning easier in case of macros, for example.

Same for the other message.


http://reviews.llvm.org/D15710