bkramer updated this revision to Diff 80574.
bkramer marked 9 inline comments as done.
bkramer added a comment.
Herald added a subscriber: JDevlieghere.
- Moved external linkage check to matcher
- added msvcrt entry point check
- fixed comment typos.
https://reviews.llvm.org/D23130
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/GlobalNamesCheck.cpp
clang-tidy/google/GlobalNamesCheck.h
clang-tidy/google/GlobalNamesInHeadersCheck.cpp
clang-tidy/google/GlobalNamesInHeadersCheck.h
clang-tidy/google/GoogleTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/google-global-names-in-headers.rst
docs/clang-tidy/checks/google-global-names.rst
test/clang-tidy/google-global-names.cpp
unittests/clang-tidy/GoogleModuleTest.cpp
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<readability::GlobalNamesInHeadersCheck>(
+ test::runCheckOnCode<readability::GlobalNamesCheck>(
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"));
}
Index: test/clang-tidy/google-global-names.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/google-global-names.cpp
@@ -0,0 +1,46 @@
+// 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() {}
+extern "C++" void h2() {}
+
+#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: 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.
+Flag global namespace pollution in header files.
+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 relevant style guide section is
https://google.github.io/styleguide/cppguide.html#Namespaces.
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
Improvements to clang-tidy
--------------------------
+- `google-global-names
+ <http://clang.llvm.org/extra/clang-tidy/checks/google-global-names.html>`_ check
+
+ Renamed from `google-global-names-in-headers`. Now also flags functions,
+ variables and classes in the global namespace.
+
- New `cppcoreguidelines-slicing
<http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
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 "NonConstReferences.h"
@@ -64,8 +64,8 @@
CheckFactories
.registerCheck<clang::tidy::readability::BracesAroundStatementsCheck>(
"google-readability-braces-around-statements");
- CheckFactories.registerCheck<readability::GlobalNamesInHeadersCheck>(
- "google-global-names-in-headers");
+ CheckFactories.registerCheck<readability::GlobalNamesCheck>(
+ "google-global-names");
CheckFactories.registerCheck<clang::tidy::readability::FunctionSizeCheck>(
"google-readability-function-size");
CheckFactories
Index: clang-tidy/google/GlobalNamesInHeadersCheck.cpp
===================================================================
--- clang-tidy/google/GlobalNamesInHeadersCheck.cpp
+++ /dev/null
@@ -1,80 +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<Decl>("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<UsingDirectiveDecl>(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/GlobalNamesCheck.h
===================================================================
--- clang-tidy/google/GlobalNamesCheck.h
+++ clang-tidy/google/GlobalNamesCheck.h
@@ -1,14 +1,14 @@
-//===--- GlobalNamesInHeadersCheck.h - clang-tidy ---------------*- C++ -*-===//
+//===--- GlobalNamesCheck.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_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,18 +18,20 @@
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
/// of header files (the filename extensions should not contain "." prefix).
/// "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,118 @@
+//===--- 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 {
+
+namespace {
+AST_MATCHER(NamedDecl, isExternallyVisible) {
+ return Node.isExternallyVisible();
+}
+} // namespace
+
+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(isExternallyVisible()),
+ functionDecl(unless(isExternC()), isExternallyVisible()),
+ varDecl(unless(isExternC()), isExternallyVisible())))
+ .bind("decl"),
+ this);
+}
+
+void GlobalNamesCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *D = Result.Nodes.getNodeAs<NamedDecl>("decl");
+
+ // If it comes from a macro, we'll assume it is fine.
+ if (D->getLocStart().isMacroID())
+ return;
+
+ if (isa<UsingDirectiveDecl>(D) || isa<UsingDecl>(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<UsingDirectiveDecl>(D)) {
+ if (UsingDirective->getNominatedNamespace()->isAnonymousNamespace()) {
+ // Anonymous 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;
+ }
+
+ if (const auto *RDecl = dyn_cast<CXXRecordDecl>(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<FunctionDecl>(D)) {
+ // main() should be in the global namespace.
+ if (FDecl->isMain() || FDecl->isMSVCRTEntryPoint())
+ 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; this can cause "
+ "ODR violations, consider moving it into a namespace")
+ << D;
+}
+
+} // namespace readability
+} // namespace google
+} // namespace tidy
+} // namespace clang
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
