Index: clang-tidy/google/CMakeLists.txt =================================================================== --- clang-tidy/google/CMakeLists.txt +++ clang-tidy/google/CMakeLists.txt @@ -5,7 +5,7 @@ DefaultArgumentsCheck.cpp ExplicitConstructorCheck.cpp ExplicitMakePairCheck.cpp - GlobalNamesInHeadersCheck.cpp + GlobalNamesCheck.cpp GoogleTidyModule.cpp IntegerTypesCheck.cpp MemsetZeroLengthCheck.cpp Index: clang-tidy/google/GlobalNamesCheck.h =================================================================== --- clang-tidy/google/GlobalNamesCheck.h +++ clang-tidy/google/GlobalNamesCheck.h @@ -1,4 +1,4 @@ -//===--- GlobalNamesInHeadersCheck.h - clang-tidy ---------------*- C++ -*-===// +//===--- GlobalNamesCheck.h - clang-tidy ------------------------*- C++ -*-===// // // The LLVM Compiler Infrastructure // @@ -7,8 +7,8 @@ // //===----------------------------------------------------------------------===// -#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESINHEADERSCHECK_H -#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESINHEADERSCHECK_H +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESCHECK_H #include "../ClangTidy.h" #include "../utils/HeaderFileExtensionsUtils.h" @@ -18,8 +18,10 @@ namespace google { namespace readability { -/// Flag global namespace pollution in header files. -/// Right now it only triggers on using declarations and directives. +/// Flag global namespace pollution. +/// Right now it only triggers on using declarations and directives in header +/// files and declarations and definitions of functions, classes and variables +/// in the global namespace. /// /// The check supports these options: /// - `HeaderFileExtensions`: a comma-separated list of filename extensions @@ -27,9 +29,9 @@ /// "h" by default. /// For extension-less header files, using an empty string or leaving an /// empty string between "," if there are other filename extensions. -class GlobalNamesInHeadersCheck : public ClangTidyCheck { +class GlobalNamesCheck : public ClangTidyCheck { public: - GlobalNamesInHeadersCheck(StringRef Name, ClangTidyContext *Context); + GlobalNamesCheck(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; @@ -44,4 +46,4 @@ } // namespace tidy } // namespace clang -#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESINHEADERSCHECK_H +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_GLOBALNAMESCHECK_H Index: clang-tidy/google/GlobalNamesCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/google/GlobalNamesCheck.cpp @@ -0,0 +1,120 @@ +//===--- GlobalNamesCheck.cpp - clang-tidy ----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "GlobalNamesCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace google { +namespace readability { + +GlobalNamesCheck::GlobalNamesCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + RawStringHeaderFileExtensions( + Options.getLocalOrGlobal("HeaderFileExtensions", "h")) { + if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions, + HeaderFileExtensions, + ',')) { + llvm::errs() << "Invalid header file extension: " + << RawStringHeaderFileExtensions << "\n"; + } +} + +void GlobalNamesCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions); +} + +void GlobalNamesCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + Finder->addMatcher( + namedDecl(hasDeclContext(translationUnitDecl()), + anyOf(usingDecl(), usingDirectiveDecl(), cxxRecordDecl(), + functionDecl(unless(isExternC())), + varDecl(unless(isExternC())))) + .bind("decl"), + this); +} + +void GlobalNamesCheck::check(const MatchFinder::MatchResult &Result) { + const auto *D = Result.Nodes.getNodeAs("decl"); + + // If it comes from a macro, we'll assume it is fine. + if (D->getLocStart().isMacroID()) + return; + + if (isa(D) || isa(D)) { + // Ignore if it comes from the "main" file ... + if (Result.SourceManager->isInMainFile( + Result.SourceManager->getExpansionLoc(D->getLocStart()))) { + // unless that file is a header. + if (!utils::isSpellingLocInHeaderFile( + D->getLocStart(), *Result.SourceManager, HeaderFileExtensions)) + return; + } + + if (const auto *UsingDirective = dyn_cast(D)) { + if (UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) { + // Anynoumous namespaces inject a using directive into the AST to import + // the names into the containing namespace. + // We should not have them in headers, but there is another warning for + // that. + return; + } + } + diag( + D->getLocStart(), + "using declarations in the global namespace in headers are prohibited"); + return; + } + + // Ignore decls with internal linkage. + if (!D->isExternallyVisible()) + return; + + if (const auto *VDecl = dyn_cast(D)) { + // extern "C" globals need to be in the global namespace. + if (VDecl->isExternC()) + return; + } + + if (const auto *RDecl = dyn_cast(D)) { + // Ignore C-like records. Those are often used in C wrappers and don't + // contain anything with linkage. + if (RDecl->isCLike()) + return; + } + + if (const auto *FDecl = dyn_cast(D)) { + // main() should be in the global namespace. + if (FDecl->isMain()) + return; + + // Ignore overloaded operators. + // FIXME: Should this only ignore global operator new/delete? + if (FDecl->isOverloadedOperator()) + return; + } + + // FIXME: If we want to be really fancy we could move declaration-less + // definitions into a global namespace or make them static with a FixIt. + diag(D->getLocation(), "%0 declared in the global namespace") << D; +} + +} // namespace readability +} // namespace google +} // namespace tidy +} // namespace clang Index: clang-tidy/google/GlobalNamesInHeadersCheck.cpp =================================================================== --- clang-tidy/google/GlobalNamesInHeadersCheck.cpp +++ /dev/null @@ -1,81 +0,0 @@ -//===--- GlobalNamesInHeadersCheck.cpp - clang-tidy -----------------*- C++ -*-===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// - -#include "GlobalNamesInHeadersCheck.h" -#include "clang/AST/ASTContext.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/ASTMatchers/ASTMatchers.h" -#include "clang/Lex/Lexer.h" - -using namespace clang::ast_matchers; - -namespace clang { -namespace tidy { -namespace google { -namespace readability { - -GlobalNamesInHeadersCheck::GlobalNamesInHeadersCheck(StringRef Name, - ClangTidyContext *Context) - : ClangTidyCheck(Name, Context), - RawStringHeaderFileExtensions( - Options.getLocalOrGlobal("HeaderFileExtensions", "h")) { - if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions, - HeaderFileExtensions, - ',')) { - llvm::errs() << "Invalid header file extension: " - << RawStringHeaderFileExtensions << "\n"; - } -} - -void GlobalNamesInHeadersCheck::storeOptions( - ClangTidyOptions::OptionMap &Opts) { - Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions); -} - -void -GlobalNamesInHeadersCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { - Finder->addMatcher( - decl(anyOf(usingDecl(), usingDirectiveDecl()), - hasDeclContext(translationUnitDecl())).bind("using_decl"), - this); -} - -void GlobalNamesInHeadersCheck::check(const MatchFinder::MatchResult &Result) { - const auto *D = Result.Nodes.getNodeAs("using_decl"); - // If it comes from a macro, we'll assume it is fine. - if (D->getLocStart().isMacroID()) - return; - - // Ignore if it comes from the "main" file ... - if (Result.SourceManager->isInMainFile( - Result.SourceManager->getExpansionLoc(D->getLocStart()))) { - // unless that file is a header. - if (!utils::isSpellingLocInHeaderFile( - D->getLocStart(), *Result.SourceManager, HeaderFileExtensions)) - return; - } - - if (const auto* UsingDirective = dyn_cast(D)) { - if (UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) { - // Anynoumous namespaces inject a using directive into the AST to import - // the names into the containing namespace. - // We should not have them in headers, but there is another warning for - // that. - return; - } - } - - diag(D->getLocStart(), - "using declarations in the global namespace in headers are prohibited"); -} - -} // namespace readability -} // namespace google -} // namespace tidy -} // namespace clang Index: clang-tidy/google/GoogleTidyModule.cpp =================================================================== --- clang-tidy/google/GoogleTidyModule.cpp +++ clang-tidy/google/GoogleTidyModule.cpp @@ -18,7 +18,7 @@ #include "DefaultArgumentsCheck.h" #include "ExplicitConstructorCheck.h" #include "ExplicitMakePairCheck.h" -#include "GlobalNamesInHeadersCheck.h" +#include "GlobalNamesCheck.h" #include "IntegerTypesCheck.h" #include "MemsetZeroLengthCheck.h" #include "OverloadedUnaryAndCheck.h" @@ -64,8 +64,8 @@ CheckFactories .registerCheck( "google-readability-braces-around-statements"); - CheckFactories.registerCheck( - "google-global-names-in-headers"); + CheckFactories.registerCheck( + "google-global-names"); CheckFactories.registerCheck( "google-readability-function-size"); CheckFactories Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -59,6 +59,11 @@ Improvements to clang-tidy -------------------------- +- `google-global-names + `_ check + + Now flags functions, variables and classes in the global namespace too. + - New `cppcoreguidelines-slicing `_ check Index: docs/clang-tidy/checks/google-global-names.rst =================================================================== --- docs/clang-tidy/checks/google-global-names.rst +++ docs/clang-tidy/checks/google-global-names.rst @@ -1,10 +1,12 @@ -.. title:: clang-tidy - google-global-names-in-headers +.. title:: clang-tidy - google-global-names -google-global-names-in-headers +google-global-names ============================== Flag global namespace pollution in header files. -Right now it only triggers on ``using`` declarations and directives. +Right now it only triggers on using declarations and directives in header files +and declarations and definitions of functions, classes and variables in the +global namespace. The check supports these options: - `HeaderFileExtensions`: a comma-separated list of filename extensions Index: test/clang-tidy/google-global-names.cpp =================================================================== --- /dev/null +++ test/clang-tidy/google-global-names.cpp @@ -0,0 +1,45 @@ +// RUN: %check_clang_tidy %s google-global-names %t + +void f(); +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'f' declared in the global namespace +void f() {} +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: 'f' declared in the global namespace +class F; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'F' declared in the global namespace +class F {}; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: 'F' declared in the global namespace +int i; +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'i' declared in the global namespace +extern int ii = 0; +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'ii' declared in the global namespace + +// No warnings below. +extern "C" void g(); +extern "C" void h() {} + +#define VAR(v) v +VAR(int m); + +extern "C" { +void j() {} +} + +struct Clike { + int i; +}; + +extern "C" int ik; +extern "C" { int il; } + +void *operator new(__SIZE_TYPE__, int) { return 0; } +void operator delete(void *, int) {} + +static void l() {} +namespace { +void m() {} +} +namespace x { +void n() {} +} + +int main() {} Index: unittests/clang-tidy/GoogleModuleTest.cpp =================================================================== --- unittests/clang-tidy/GoogleModuleTest.cpp +++ unittests/clang-tidy/GoogleModuleTest.cpp @@ -1,6 +1,6 @@ #include "ClangTidyTest.h" #include "google/ExplicitConstructorCheck.h" -#include "google/GlobalNamesInHeadersCheck.h" +#include "google/GlobalNamesCheck.h" #include "gtest/gtest.h" using namespace clang::tidy::google; @@ -56,7 +56,7 @@ "A(Foo);")); } -class GlobalNamesInHeadersCheckTest : public ::testing::Test { +class GlobalNamesCheckTest : public ::testing::Test { protected: bool runCheckOnCode(const std::string &Code, const std::string &Filename) { static const char *const Header = "namespace std {\n" @@ -69,7 +69,7 @@ if (!StringRef(Filename).endswith(".cpp")) { Args.emplace_back("-xc++-header"); } - test::runCheckOnCode( + test::runCheckOnCode( Header + Code, &Errors, Filename, Args); if (Errors.empty()) return false; @@ -81,7 +81,7 @@ } }; -TEST_F(GlobalNamesInHeadersCheckTest, UsingDeclarations) { +TEST_F(GlobalNamesCheckTest, UsingDeclarations) { EXPECT_TRUE(runCheckOnCode("using std::string;", "foo.h")); EXPECT_FALSE(runCheckOnCode("using std::string;", "foo.cpp")); EXPECT_FALSE(runCheckOnCode("namespace my_namespace {\n" @@ -91,7 +91,7 @@ EXPECT_FALSE(runCheckOnCode("SOME_MACRO(std::string);", "foo.h")); } -TEST_F(GlobalNamesInHeadersCheckTest, UsingDirectives) { +TEST_F(GlobalNamesCheckTest, UsingDirectives) { EXPECT_TRUE(runCheckOnCode("using namespace std;", "foo.h")); EXPECT_FALSE(runCheckOnCode("using namespace std;", "foo.cpp")); EXPECT_FALSE(runCheckOnCode("namespace my_namespace {\n" @@ -101,7 +101,7 @@ EXPECT_FALSE(runCheckOnCode("SOME_MACRO(namespace std);", "foo.h")); } -TEST_F(GlobalNamesInHeadersCheckTest, RegressionAnonymousNamespace) { +TEST_F(GlobalNamesCheckTest, RegressionAnonymousNamespace) { EXPECT_FALSE(runCheckOnCode("namespace {}", "foo.h")); }