Index: clang-tidy/google/CMakeLists.txt =================================================================== --- clang-tidy/google/CMakeLists.txt +++ clang-tidy/google/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyGoogleModule AvoidCStyleCastsCheck.cpp + DefinitionsInHeadersCheck.cpp ExplicitConstructorCheck.cpp ExplicitMakePairCheck.cpp GlobalNamesInHeadersCheck.cpp Index: clang-tidy/google/DefinitionsInHeadersCheck.h =================================================================== --- /dev/null +++ clang-tidy/google/DefinitionsInHeadersCheck.h @@ -0,0 +1,33 @@ +//===--- DefinitionsInHeadersCheck.h - clang-tidy----------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_DEFINITIONS_IN_HEADERS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_DEFINITIONS_IN_HEADERS_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace google { + +// Finds non-inline function definition and variable definition in headers, +// which may violate cpp one definition rule. +class DefinitionsInHeadersCheck : public ClangTidyCheck { +public: + DefinitionsInHeadersCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace google +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_DEFINITIONS_IN_HEADERS_H Index: clang-tidy/google/DefinitionsInHeadersCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/google/DefinitionsInHeadersCheck.cpp @@ -0,0 +1,94 @@ +//===--- DefinitionsInHeadersCheck.cpp - clang-tidy------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "DefinitionsInHeadersCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace google { + +namespace { + +bool inHeaderFile(const SourceManager* SM, + const SourceLocation& Location) { + StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location)); + return Filename.endswith(".h"); +} + +} // namespace + +void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(functionDecl(isDefinition()).bind("fun-definition"), this); + Finder->addMatcher(varDecl(isDefinition()).bind("var-definition"), this); +} + +void DefinitionsInHeadersCheck::check(const MatchFinder::MatchResult &Result) { + // There can be more than one definition of a class type, enumeration type, + // inline function with external linkage, class template, non-static function + // template, static data member of a class template, member function of a + // class template, or template specialization for which some template + // parameters are not specifiedin a program provided that each definition + // appears in a different translation unit, and provided the definitions + // satisfy the following requirements. (C++ basic.def.odr) + if (auto funDecl = Result.Nodes.getNodeAs("fun-definition")) { + if (!inHeaderFile(Result.SourceManager, funDecl->getLocStart())) + return; + + if (funDecl->getLinkageInternal() == InternalLinkage || + funDecl->getLinkageInternal() == UniqueExternalLinkage) + return; + + // Inline function is allowed. + if (funDecl->isInlined()) + return; + // Function template is allowed. + if (funDecl->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate) + return; + // Function template full specialization is prohibited in header file. + if (funDecl->getTemplateSpecializationKind() == TSK_ImplicitInstantiation) + return; + // Member function of a class template is allowed. + if (auto cxxMethodDecl = dyn_cast(funDecl)) { + if (cxxMethodDecl->getParent()->getDescribedClassTemplate()) { + return; + } + } + + diag(funDecl->getLocation(), + "function definition is not allowed in header file.") + << FixItHint::CreateInsertion(funDecl->getSourceRange().getBegin(), + "inline "); + } else if (auto varDecl = Result.Nodes.getNodeAs("var-definition")) { + if (!inHeaderFile(Result.SourceManager, varDecl->getLocStart())) + return; + + // Internal linkage variable(const, static, anonymous namespace) is allowed: + if (varDecl->getLinkageInternal() == InternalLinkage || + varDecl->getLinkageInternal() == UniqueExternalLinkage) + return; + // static data member of a class template is allowed. + if (varDecl->getDeclContext()->isDependentContext() && + varDecl->isStaticDataMember()) + return; + // Ignore variable definition within function scope. + if (varDecl->hasLocalStorage() || varDecl->isStaticLocal()) + return; + + diag(varDecl->getLocation(), + "variable definition is not allowed in header file."); + } +} + +} // namespace google +} // namespace tidy +} // namespace clang Index: clang-tidy/google/GoogleTidyModule.cpp =================================================================== --- clang-tidy/google/GoogleTidyModule.cpp +++ clang-tidy/google/GoogleTidyModule.cpp @@ -15,6 +15,7 @@ #include "../readability/NamespaceCommentCheck.h" #include "../readability/RedundantSmartptrGetCheck.h" #include "AvoidCStyleCastsCheck.h" +#include "DefinitionsInHeadersCheck.h" #include "ExplicitConstructorCheck.h" #include "ExplicitMakePairCheck.h" #include "GlobalNamesInHeadersCheck.h" @@ -35,6 +36,8 @@ class GoogleModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "google-definitions-in-headers"); CheckFactories.registerCheck( "google-build-explicit-make-pair"); CheckFactories.registerCheck( Index: docs/clang-tidy/checks/google-definitions-in-headers.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/google-definitions-in-headers.rst @@ -0,0 +1,5 @@ +google-definitions-in-headers +============================= + +Finds non-inline function definition and variable definition in headers, which +may violate cpp one definition rule. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -17,6 +17,7 @@ google-build-explicit-make-pair google-build-namespaces google-build-using-namespace + google-definitions-in-headers google-explicit-constructor google-global-names-in-headers google-readability-braces-around-statements Index: unittests/clang-tidy/GoogleModuleTest.cpp =================================================================== --- unittests/clang-tidy/GoogleModuleTest.cpp +++ unittests/clang-tidy/GoogleModuleTest.cpp @@ -1,4 +1,5 @@ #include "ClangTidyTest.h" +#include "google/DefinitionsInHeadersCheck.h" #include "google/ExplicitConstructorCheck.h" #include "google/GlobalNamesInHeadersCheck.h" #include "gtest/gtest.h" @@ -105,6 +106,142 @@ EXPECT_FALSE(runCheckOnCode("namespace {}", "foo.h")); } +class DefinitionsInHeadersCheckTest : public ::testing::Test { +protected: + std::string runCheckOnCode(const std::string &Code, + const std::string &Filename) { + std::vector Errors; + std::vector Args; + if (!StringRef(Filename).endswith(".cpp")) { + Args.emplace_back("-xc++-header"); + } + test::runCheckOnCode( + Code, &Errors, Filename, Args); + if (Errors.empty()) + return ""; + return Errors[0].Message.Message; + } +}; + +TEST_F(DefinitionsInHeadersCheckTest, FunctionDefinition) { + const std::string errorMessage = + "function definition is not allowed in header file."; + const std::string okMessage = ""; + EXPECT_EQ(errorMessage, runCheckOnCode("int f() { return 0; }", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("int f();\n" + "int f() { return 1; }", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("class A {\n" + " int f();\n" + "};\n" + "int A::f() { return 1; }", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("namespace A {\n" + " int f() { return 1; }\n" + "}", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("template \n" + "T f() {\n" + " T a = 1;\n" + " return a;\n" + "}\n" + "template <>\n" + "int f() {\n" + " int a = 1;\n" + " return a;\n" + "}", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("struct A {\n" + " template\n" + " T f() {\n" + " T a = 1;\n" + " return a;\n" + " }\n" + "};\n" + "template<>\n" + "int A::f() {\n" + " int a = 1;\n" + " return a;\n" + "}", "foo.h")); + + EXPECT_EQ(okMessage, runCheckOnCode("int f();", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("inline int f() { return 1; }", + "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("namespace {\n" + " int f() { return 1; }\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("int f() { return 1; }\n", + "foo.cc")); + EXPECT_EQ(okMessage, runCheckOnCode("class A {\n" + " int f() { return 1; }\n" + "};", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("struct A {\n" + " template\n" + " T f() {\n" + " T a = 1;\n" + " return a;\n" + " }\n" + "};\n", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template\n" + "struct A {\n" + " static void f();\n" + "};\n" + "template \n" + "void A::f() {\n" + " T a;\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template\n" + "struct A {\n" + " void f();\n" + "};\n" + "template \n" + "void A::f() {\n" + " T a;\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template \n" + "T f() {\n" + " T a;\n" + " return a;\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template \n" + "void fun(T) {}", "foo.h")); +} + +TEST_F(DefinitionsInHeadersCheckTest, VariableDefinition) { + const std::string errorMessage = + "variable definition is not allowed in header file."; + const std::string okMessage = ""; + EXPECT_EQ(errorMessage, runCheckOnCode("int a;", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("class A{};\n" + "A a;", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("namespace A {\n" + " int a = 1;\n" + "}", "foo.h")); + EXPECT_EQ(errorMessage, runCheckOnCode("struct A {\n" + " static int a;\n" + "};\n" + "int A::a = 1;", "foo.h")); + + EXPECT_EQ(okMessage, runCheckOnCode("namespace {\n" + " int a = 1;\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("static int a = 1;", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("const int a = 1;", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("class A { int b; };", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("class A { };", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("class A;", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("extern int a;", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("struct A {\n" + " int f() {\n" + " static int a = 1;\n" + " int b = ++a;" + " return b;\n" + " }\n" + "};", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template\n" + "struct A {\n" + " static const int a;\n" + "};\n" + "template\n" + "const int A::a = 1;", "foo.h")); +} + } // namespace test } // namespace tidy } // namespace clang