diff options
9 files changed, 350 insertions, 1 deletions
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 17ee66c7987..ea96278956c 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -5,6 +5,7 @@ add_clang_library(clangTidyMiscModule AssertSideEffectCheck.cpp AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversionCheck.cpp + DefinitionsInHeadersCheck.cpp InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp MacroParenthesesCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp new file mode 100644 index 00000000000..29fa0f6d5bf --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.cpp @@ -0,0 +1,126 @@ +//===--- 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 { + +AST_MATCHER(NamedDecl, isHeaderFileExtension) { + SourceManager& SM = Finder->getASTContext().getSourceManager(); + SourceLocation ExpansionLoc = SM.getExpansionLoc(Node.getLocStart()); + StringRef Filename = SM.getFilename(ExpansionLoc); + return Filename.endswith(".h") || Filename.endswith(".hh") || + Filename.endswith(".hpp") || Filename.endswith(".hxx") || + llvm::sys::path::extension(Filename).empty(); +} + +} // namespace + +DefinitionsInHeadersCheck::DefinitionsInHeadersCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + UseHeaderFileExtension(Options.get("UseHeaderFileExtension", true)) {} + +void DefinitionsInHeadersCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "UseHeaderFileExtension", UseHeaderFileExtension); +} + +void DefinitionsInHeadersCheck::registerMatchers(MatchFinder *Finder) { + if (UseHeaderFileExtension) { + Finder->addMatcher( + namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())), + isHeaderFileExtension()).bind("name-decl"), + this); + } else { + Finder->addMatcher( + namedDecl(anyOf(functionDecl(isDefinition()), varDecl(isDefinition())), + anyOf(isHeaderFileExtension(), + unless(isExpansionInMainFile()))).bind("name-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>("name-decl"); + assert(ND); + + // Internal linkage variable definitions are ignored for now: + // const int a = 1; + // static int b = 1; + // + // Although these might also cause ODR violations, we can be less certain and + // should try to keep the false-positive rate down. + if (ND->getLinkageInternal() == InternalLinkage) + return; + + if (const auto *FD = dyn_cast<FunctionDecl>(ND)) { + // Inline functions are allowed. + if (FD->isInlined()) + return; + // Function templates are 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 '%0' defined in a header file; " + "function definitions in header files can lead to ODR violations") + << FD->getNameInfo().getName().getAsString() + << FixItHint::CreateInsertion(FD->getSourceRange().getBegin(), + "inline "); + } else if (const auto *VD = dyn_cast<VarDecl>(ND)) { + // Static data members of a class template are 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 '%0' defined in a header file; " + "variable definitions in header files can lead to ODR violations") + << VD->getName(); + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h new file mode 100644 index 00000000000..93956099b1b --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/DefinitionsInHeadersCheck.h @@ -0,0 +1,43 @@ +//===--- 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. +// +// There is one option: +// - `UseHeaderFileExtension`: Whether to use file extension (h, hh, hpp, hxx) +// to distinguish header files. True by default. +// +// 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); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + const bool UseHeaderFileExtension; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_DEFINITIONS_IN_HEADERS_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 8d8de17724e..ec11374a419 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/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" @@ -48,6 +49,8 @@ public: "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>( diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 20bc8063b68..f72ea4b392e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -40,6 +40,7 @@ Clang-Tidy Checks 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 diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-definitions-in-headers.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-definitions-in-headers.rst new file mode 100644 index 00000000000..c7bf3131d54 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-definitions-in-headers.rst @@ -0,0 +1,37 @@ +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; // Warning. + extern int d; // OK: extern variable. + + namespace N { + int e = 2; // Warning. + } + + // Internal linkage variable definitions are ignored for now. + // Although these might also cause ODR violations, we can be less certain and + // should try to keep the false-positive rate down. + static int b = 1; + const int c = 1; + + // Warning. + int g() { + return 1; + } + + // OK: inline function definition. + inline int e() { + return 1; + } + + class A { + public: + int f1() { return 1; } // OK: inline member function definition. + int f2(); + }; + + int A::f2() { return 1; } // Warning. diff --git a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py index 9a6dfdc15bb..0feb4b4aea3 100755 --- a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py +++ b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py @@ -52,6 +52,8 @@ def main(): extension = '.cpp' if (input_file_name.endswith('.c')): extension = '.c' + if (input_file_name.endswith('.hpp')): + extension = '.hpp' temp_file_name = temp_file_name + extension clang_tidy_extra_args = extra_args diff --git a/clang-tools-extra/test/clang-tidy/misc-definitions-in-headers.hpp b/clang-tools-extra/test/clang-tidy/misc-definitions-in-headers.hpp new file mode 100644 index 00000000000..29d999917be --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-definitions-in-headers.hpp @@ -0,0 +1,135 @@ +// RUN: %check_clang_tidy %s misc-definitions-in-headers %t + +int f() { +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f' defined in a header file; function definitions in header files can lead to ODR violations [misc-definitions-in-headers] +// CHECK-FIXES: inline int f() { + return 1; +} + +class CA { + void f1() {} // OK: inline class member function definition. + void f2(); + template<typename T> + T f3() { + T a = 1; + return a; + } + template<typename T> + struct CAA { + struct CAB { + void f4(); + }; + }; +}; + +void CA::f2() { } +// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: function 'f2' defined in a header file; +// CHECK-FIXES: inline void CA::f2() { + +template <> +int CA::f3() { +// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: function 'f3' defined in a header file; + int a = 1; + return a; +} + +template <typename T> +void CA::CAA<T>::CAB::f4() { +// OK: member function definition of a nested template class in a class. +} + +template <typename T> +struct CB { + void f1(); + struct CCA { + void f2(T a); + }; + struct CCB; // OK: forward declaration. + static int a; // OK: class static data member declaration. +}; + +template <typename T> +void CB<T>::f1() { // OK: Member function definition of a class template. +} + +template <typename T> +void CB<T>::CCA::f2(T a) { +// OK: member function definition of a nested class in a class template. +} + +template <typename T> +struct CB<T>::CCB { + void f3(); +}; + +template <typename T> +void CB<T>::CCB::f3() { +// OK: member function definition of a nested class in a class template. +} + +template <typename T> +int CB<T>::a = 2; // OK: static data member definition of a class template. + +template <typename T> +T tf() { // OK: template function definition. + T a; + return a; +} + + +namespace NA { + int f() { return 1; } +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function 'f' defined in a header file; +// CHECK-FIXES: inline int f() { return 1; } +} + +template <typename T> +T f3() { + T a = 1; + return a; +} + +template <> +// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: function 'f3' defined in a header file; +int f3() { + int a = 1; + return a; +} + +int f5(); // OK: function declaration. +inline int f6() { return 1; } // OK: inline function definition. +namespace { + int f7() { return 1; } +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: function 'f7' defined in a header file; +} + +int a = 1; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: variable 'a' defined in a header file; variable definitions in header files can lead to ODR violations [misc-definitions-in-headers] +CA a1; +// CHECK-MESSAGES: :[[@LINE-1]]:4: warning: variable 'a1' defined in a header file; + +namespace NB { + int b = 1; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'b' defined in a header file; + const int c = 1; // OK: internal linkage variable definition. +} + +class CC { + static int d; // OK: class static data member declaration. +}; + +int CC::d = 1; +// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: variable 'd' defined in a header file; + +const char* ca = "foo"; +// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: variable 'ca' defined in a header file; + +namespace { + int e = 2; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: variable 'e' defined in a header file; +} + +const char* const g = "foo"; // OK: internal linkage variable definition. +static int h = 1; // OK: internal linkage variable definition. +const int i = 1; // OK: internal linkage variable definition. +extern int j; // OK: internal linkage variable definition. diff --git a/clang-tools-extra/test/lit.cfg b/clang-tools-extra/test/lit.cfg index e6dab49377d..5344e391e7e 100644 --- a/clang-tools-extra/test/lit.cfg +++ b/clang-tools-extra/test/lit.cfg @@ -43,7 +43,8 @@ else: config.test_format = lit.formats.ShTest(execute_external) # suffixes: A list of file extensions to treat as test files. -config.suffixes = ['.c', '.cpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s', '.modularize', '.module-map-checker'] +config.suffixes = ['.c', '.cpp', '.hpp', '.m', '.mm', '.cu', '.ll', '.cl', '.s', + '.modularize', '.module-map-checker'] # Test-time dependencies located in directories called 'Inputs' are excluded # from test suites; there won't be any lit tests within them. |
