hokein updated this revision to Diff 43686.
hokein added a comment.
Address aaron's comments.
http://reviews.llvm.org/D15710
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/DefinitionsInHeadersCheck.cpp
clang-tidy/misc/DefinitionsInHeadersCheck.h
clang-tidy/misc/MiscTidyModule.cpp
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-definitions-in-headers.rst
unittests/clang-tidy/MiscModuleTest.cpp
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<ClangTidyError> Errors;
+ std::vector<std::string> Args;
+ if (!StringRef(Filename).endswith(".cpp")) {
+ Args.emplace_back("-xc++-header");
+ }
+ test::runCheckOnCode<misc::DefinitionsInHeadersCheck>(
+ 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 <typename T>\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<typename T>\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<typename T>\n"
+ " T f() {\n"
+ " T a = 1;\n"
+ " return a;\n"
+ " }\n"
+ "};\n", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template<typename T>\n"
+ "struct A {\n"
+ " static void f();\n"
+ "};\n"
+ "template <typename T>\n"
+ "void A<T>::f() {\n"
+ " T a;\n"
+ "}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template<typename T>\n"
+ "struct A {\n"
+ " void f();\n"
+ "};\n"
+ "template <typename T>\n"
+ "void A<T>::f() {\n"
+ " T a;\n"
+ "}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template <typename T>\n"
+ "T f() {\n"
+ " T a;\n"
+ " return a;\n"
+ "}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template <typename T>\n"
+ "void fun(T) {}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template<typename T>\n"
+ "struct A {\n"
+ " struct B {"
+ " void add(T a);"
+ " };"
+ " T a;"
+ "};\n"
+ "template <typename T>\n"
+ "void A<T>::B::add(T a) {\n"
+ "}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("struct A {\n"
+ "template<typename T>\n"
+ " struct B {"
+ " struct C {"
+ " void add(T a);"
+ " };"
+ " };"
+ "};\n"
+ "template <typename T>\n"
+ "void A::B<T>::C::add(T a) {\n"
+ "}", "foo.h"));
+ EXPECT_EQ(okMessage, runCheckOnCode("template <typename T>\n"
+ "struct A {\n"
+ " struct B;\n"
+ "};\n"
+ "template <typename T>\n"
+ "struct A<T>::B {\n"
+ " int f();\n"
+ "};\n"
+ "template <typename T>\n"
+ "int A<T>::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<typename T>\n"
+ "struct A {\n"
+ " static const int a;\n"
+ "};\n"
+ "template<typename T>\n"
+ "const int A<T>::a = 1;", "foo.h"));
+}
+
} // namespace test
} // namespace tidy
} // namespace clang
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: 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: 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<BoolPointerImplicitConversionCheck>(
"misc-bool-pointer-implicit-conversion");
+ CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
+ "misc-definitions-in-headers");
CheckFactories.registerCheck<InaccurateEraseCheck>(
"misc-inaccurate-erase");
CheckFactories.registerCheck<InefficientAlgorithmCheck>(
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<NamedDecl>("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<FunctionDecl>(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<CXXMethodDecl>(FD)) {
+ const auto *DC = MD->getDeclContext();
+ while (DC->isRecord()) {
+ if (const auto *RD = dyn_cast<CXXRecordDecl>(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<VarDecl>(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/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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
