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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:22
@@ +21,3 @@
+
+bool inHeaderFile(const SourceManager* SM, SourceLocation Location) {
+  StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location));
----------------
This should use isInMainFile() instead of checking the file extension.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:30
@@ +29,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      decl(anyOf(functionDecl(isDefinition()),
----------------
I wonder if you could use an AST matcher to weed out definitions that are in the main file to make the matcher a bit more narrow.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:36
@@ +35,3 @@
+void DefinitionsInHeadersCheck::check(const MatchFinder::MatchResult &Result) {
+  // C++ [basic.def.odr]:
+  // There can be more than one definition of a class type, enumeration type,
----------------
Please quote the paragraph as well (p6, in this case).

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:44
@@ +43,3 @@
+  // satisfy the following requirements.
+  const auto* ND = Result.Nodes.getNodeAs<NamedDecl>("decl");
+  if (!ND)
----------------
Should be declared: `const auto *ND` (note the position of the *). May want to run clang-format over your patch, as this applies elsewhere as well.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:49
@@ +48,3 @@
+    return;
+  // Internal linkage variable and function definitions are allowed:
+  //   const int a = 1;
----------------
Why are these allowed? They can result in ODR violations if they are in a header file.

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:69
@@ +68,3 @@
+      return;
+    // Member function of a class template and member function of a nest class
+    // in a class template are allowed.
----------------
nest -> nested

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:77
@@ +76,3 @@
+            return;
+        DC = DC->getLookupParent();
+      }
----------------
Why the lookup parent instead of getParent()? Also, can this ever return a nullptr?

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.cpp:85
@@ +84,3 @@
+  } else if (const auto* VD = dyn_cast<VarDecl>(ND)) {
+    // static data member of a class template is allowed.
+    if (VD->getDeclContext()->isDependentContext() &&
----------------
static -> Static

================
Comment at: clang-tidy/misc/DefinitionsInHeadersCheck.h:19
@@ +18,3 @@
+
+// Finds non-extern non-inline function dand variable definitions in header
+// files, which can lead to potential ODR violations.
----------------
dand -> and

================
Comment at: docs/clang-tidy/checks/misc-definitions-in-headers.rst:17
@@ +16,3 @@
+   namespace {
+     int c  = 2; // ok
+   }
----------------
I don't think that this should be okay (same with static/const int examples above). See https://www.securecoding.cert.org/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file for more details.

================
Comment at: unittests/clang-tidy/MiscModuleTest.cpp:38
@@ -36,1 +37,3 @@
 
+class DefinitionsInHeadersCheckTest : public ::testing::Test {
+protected:
----------------
Is there a reason this is under unittests instead of as actual clang-tidy test files? I would prefer to see this run through clang-tidy on the shell instead of in the unit tests, unless there's something we cannot otherwise test.


http://reviews.llvm.org/D15710