Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -5,6 +5,7 @@ AssertSideEffectCheck.cpp AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversionCheck.cpp + DefinitionsInHeadersCheck.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp MacroParenthesesCheck.cpp Index: clang-tidy/misc/DefinitionsInHeadersCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/DefinitionsInHeadersCheck.h @@ -0,0 +1,35 @@ +//===--- 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_MISC_DEFINITIONS_IN_HEADERS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +// Finds non-extern non-inline function and variable definitions in header +// files, which can lead to potential ODR violations. +// For the user-facing documentation see: +// http://clang.llvm.org/extra/clang-tidy/checks/misc-definitions-in-headers.html +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 misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H Index: clang-tidy/misc/DefinitionsInHeadersCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/DefinitionsInHeadersCheck.cpp @@ -0,0 +1,103 @@ +//===--- 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 misc { + +namespace { + +bool inHeaderFile(const SourceManager *SM, SourceLocation Location) { + StringRef Filename = SM->getFilename(SM->getExpansionLoc(Location)); + return Filename.endswith(".h") || Filename.endswith(".hh") || + Filename.endswith(".hpp") || Filename.endswith(".hxx"); +} + +} // namespace + +void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + decl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition()))) + .bind("decl"), + this); +} + +void DefinitionsInHeadersCheck::check(const MatchFinder::MatchResult &Result) { + // C++ [basic.def.odr] p6: + // 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. + const auto *ND = Result.Nodes.getNodeAs("decl"); + if (!ND) + return; + if (!inHeaderFile(Result.SourceManager, ND->getLocStart())) + return; + // Internal linkage variable and function definitions are allowed: + // const int a = 1; + // static int a = 1; + // namespace { + // int a = 1; + // int f() { return 1} + // } + if (ND->getLinkageInternal() == InternalLinkage || + ND->getLinkageInternal() == UniqueExternalLinkage) + return; + if (const auto *FD = dyn_cast(ND)) { + // Inline function is allowed. + if (FD->isInlined()) + return; + // Function template is allowed. + if (FD->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate) + return; + // Function template full specialization is prohibited in header file. + if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation) + return; + // Member function of a class template and member function of a nested class + // in a class template are allowed. + if (const auto *MD = dyn_cast(FD)) { + const auto *DC = MD->getDeclContext(); + while (DC->isRecord()) { + if (const auto *RD = dyn_cast(DC)) + if (RD->getDescribedClassTemplate()) + return; + DC = DC->getParent(); + } + } + + diag(FD->getLocation(), "function definition is not allowed in header file") + << FixItHint::CreateInsertion(FD->getSourceRange().getBegin(), + "inline "); + } else if (const auto *VD = dyn_cast(ND)) { + // Static data member of a class template is allowed. + if (VD->getDeclContext()->isDependentContext() && VD->isStaticDataMember()) + return; + if (VD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation) + return; + // Ignore variable definition within function scope. + if (VD->hasLocalStorage() || VD->isStaticLocal()) + return; + + diag(VD->getLocation(), + "variable definition is not allowed in header file"); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -14,6 +14,7 @@ #include "AssertSideEffectCheck.h" #include "AssignOperatorSignatureCheck.h" #include "BoolPointerImplicitConversionCheck.h" +#include "DefinitionsInHeadersCheck.h" #include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" #include "MacroParenthesesCheck.h" @@ -47,6 +48,8 @@ "misc-assign-operator-signature"); CheckFactories.registerCheck( "misc-bool-pointer-implicit-conversion"); + CheckFactories.registerCheck( + "misc-definitions-in-headers"); CheckFactories.registerCheck( "misc-inaccurate-erase"); CheckFactories.registerCheck( Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -37,6 +37,7 @@ misc-assert-side-effect misc-assign-operator-signature misc-bool-pointer-implicit-conversion + misc-definitions-in-headers misc-inaccurate-erase misc-inefficient-algorithm misc-macro-parentheses Index: docs/clang-tidy/checks/misc-definitions-in-headers.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/misc-definitions-in-headers.rst @@ -0,0 +1,36 @@ +misc-definitions-in-headers +============================= + +Finds non-extern non-inline function and variable definitions in header files, which can lead to potential ODR violations. + +.. code:: c++ + // Foo.h + int a = 1; // error + static int a = 1; // ok + const int a = 1; // ok + extern int a; // ok + + namespace N { + int b = 2; // error + } + namespace { + int c = 2; // ok + } + + // error + int f() { + return 1; + } + + // ok + inline int f() { + return 1; + } + + class A { + public: + int f1() { return 1; } // ok + int f2(); + }; + + int A::f2() { return 1; } // error Index: unittests/clang-tidy/MiscModuleTest.cpp =================================================================== --- unittests/clang-tidy/MiscModuleTest.cpp +++ unittests/clang-tidy/MiscModuleTest.cpp @@ -1,5 +1,6 @@ #include "ClangTidyTest.h" #include "misc/ArgumentCommentCheck.h" +#include "misc/DefinitionsInHeadersCheck.h" #include "gtest/gtest.h" namespace clang { @@ -34,6 +35,180 @@ "void f(int xxx, int yyy); void g() { f(/*xxy=*/0, 0); }"); } +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")); + EXPECT_EQ(okMessage, runCheckOnCode("template\n" + "struct A {\n" + " struct B {" + " void add(T a);" + " };" + " T a;" + "};\n" + "template \n" + "void A::B::add(T a) {\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("struct A {\n" + "template\n" + " struct B {" + " struct C {" + " void add(T a);" + " };" + " };" + "};\n" + "template \n" + "void A::B::C::add(T a) {\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("template \n" + "struct A {\n" + " struct B;\n" + "};\n" + "template \n" + "struct A::B {\n" + " int f();\n" + "};\n" + "template \n" + "int A::B::f() {\n" + " return 1;\n" + "}", "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(errorMessage, runCheckOnCode("const char* a = \"a\";", "foo.h")); + + EXPECT_EQ(okMessage, runCheckOnCode("namespace {\n" + " int a = 1;\n" + "}", "foo.h")); + EXPECT_EQ(okMessage, runCheckOnCode("const char* const a = \"a\";", "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("namespace A {\n" + " const int a = 1;\n" + "}", "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