[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/google/DefinitionsInHeadersCheck.cpp:23
@@ +22,3 @@
+bool inHeaderFile(const SourceManager* SM,
+                  const SourceLocation& Location) {
+  StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location));
----------------
`SourceLocations` are small and trivially-copyable, they should be passed by value.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:31
@@ +30,3 @@
+void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(isDefinition()).bind("fun-definition"), this);
+  Finder->addMatcher(varDecl(isDefinition()).bind("var-definition"), this);
----------------
I'd use a single matcher for both functions and variables (`decl(anyOf(functionDecl(), varDecl()), isDefinition()).bind("decl")`) to avoid duplicating code in the callback.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:42
@@ +41,3 @@
+  // appears in a different translation unit, and provided the definitions
+  // satisfy the following requirements. (C++ basic.def.odr)
+  if (auto funDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun-definition")) {
----------------
Please put the `C++ [basic.def.odr]:` part before the quotation from the standard.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:43
@@ +42,3 @@
+  // satisfy the following requirements. (C++ basic.def.odr)
+  if (auto funDecl = Result.Nodes.getNodeAs<FunctionDecl>("fun-definition")) {
+    if (!inHeaderFile(Result.SourceManager, funDecl->getLocStart()))
----------------
Please use `const auto *` to make it clear that this is a pointer. Same elsewhere.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:52
@@ +51,3 @@
+    // Inline function is allowed.
+    if (funDecl->isInlined())
+      return;
----------------
This check can be done in the matcher.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:62
@@ +61,3 @@
+    if (auto cxxMethodDecl = dyn_cast<CXXMethodDecl>(funDecl)) {
+      if (cxxMethodDecl->getParent()->getDescribedClassTemplate()) {
+        return;
----------------
nit: No braces needed here.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.cpp:68
@@ +67,3 @@
+    diag(funDecl->getLocation(),
+         "function definition is not allowed in header file.")
+        << FixItHint::CreateInsertion(funDecl->getSourceRange().getBegin(),
----------------
Warning messages are not full sentences. No trailing period is needed.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:17
@@ +16,3 @@
+namespace tidy {
+namespace google {
+
----------------
This check is generic enough. It should go to the `misc` module, IMO.

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:19
@@ +18,3 @@
+
+// Finds non-inline function definition and variable definition in headers,
+// which may violate cpp one definition rule.
----------------
This would read slightly better: "Finds non-extern non-inline function and variable definitions in header files, which can lead to ODR violations."

================
Comment at: clang-tidy/google/DefinitionsInHeadersCheck.h:20
@@ +19,3 @@
+// Finds non-inline function definition and variable definition in headers,
+// which may violate cpp one definition rule.
+class DefinitionsInHeadersCheck : public ClangTidyCheck {
----------------
Please add a link to the user-facing documentation:

  // For the user-facing documentation see:
  // http://clang.llvm.org/extra/clang-tidy/checks/<check-name>.html

================
Comment at: docs/clang-tidy/checks/google-definitions-in-headers.rst:4
@@ +3,3 @@
+
+Finds non-inline function definition and variable definition in headers, which
+may violate cpp one definition rule.
----------------
Please update the wording to match the class comment, and also add a couple of examples of what cases does the check detect.

================
Comment at: unittests/clang-tidy/GoogleModuleTest.cpp:140
@@ +139,3 @@
+                                         "}", "foo.h"));
+  EXPECT_EQ(errorMessage, runCheckOnCode("template <typename T>\n"
+                                         "T f() {\n"
----------------
I think, we can already use raw string literals (at least, MSVC 2013 seems to support them). Raw string literals would make the test cases more readable, imo.


Repository:
  rL LLVM

http://reviews.llvm.org/D15710