Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -27,6 +27,7 @@ UnusedRAIICheck.cpp UniqueptrResetReleaseCheck.cpp VirtualNearMissCheck.cpp + ForwardDeclarationNamespaceCheck.cpp LINK_LIBS clangAST Index: clang-tidy/misc/ForwardDeclarationNamespaceCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/ForwardDeclarationNamespaceCheck.h @@ -0,0 +1,42 @@ +//===--- ForwardDeclarationNamespaceCheck.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 THIRD_PARTY_LLVM_LLVM_TOOLS_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H_ +#define THIRD_PARTY_LLVM_LLVM_TOOLS_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H_ + +#include +#include +#include +#include +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace misc { + +class ForwardDeclarationNamespaceCheck : public ClangTidyCheck { +public: + ForwardDeclarationNamespaceCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void onEndOfTranslationUnit() override; +private: + std::map> + DeclNameToDefinitions; + std::map> + DeclNameToDeclarations; + std::set FriendTypes; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // THIRD_PARTY_LLVM_LLVM_TOOLS_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H_ Index: clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp @@ -0,0 +1,169 @@ +//===--- ForwardDeclarationNamespaceCheck.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 "ForwardDeclarationNamespaceCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclBase.h" +#include "clang/AST/DeclCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace misc { + +void ForwardDeclarationNamespaceCheck::registerMatchers(MatchFinder *Finder) { + // Matches all class declarations/definitions *EXCEPT* + // 1. implicit class. Eg. class A {}; has implicit class A declared inside A + // 2. classes declared/defined inside classes(can be detected by compiler) + // 3. template class declaration, template instantiation or + // specialization(NOTE: extern specialization is filtered out by + // "unless(hasAncestor(cxxRecordDecl()))" + auto IsInSpecialization = hasAncestor( + decl(anyOf(cxxRecordDecl(isExplicitTemplateSpecialization()), + functionDecl(isExplicitTemplateSpecialization())))); + Finder->addMatcher( + cxxRecordDecl( + hasParent(decl(anyOf(namespaceDecl(), translationUnitDecl()))), + unless(isImplicit()), unless(hasAncestor(cxxRecordDecl())), + unless(isInstantiated()), unless(IsInSpecialization), + unless(classTemplateSpecializationDecl())) + .bind("record_decl"), + this); + // Match all friend declaration. Type used in friendDecl is not marked as + // referenced in AST. We need to record all record type used in friend decls. + Finder->addMatcher(friendDecl().bind("friend_decl"), this); +} + +void ForwardDeclarationNamespaceCheck::check( + const MatchFinder::MatchResult &Result) { + if (auto *recordDecl = Result.Nodes.getNodeAs("record_decl")) { + std::string declName(recordDecl->getName().data()); + if (recordDecl->isThisDeclarationADefinition()) { + DeclNameToDefinitions[declName].push_back(recordDecl); + } else { + // if decl has no definition, the definition could be in another namespace + // NOTE: even a declaration does have definition, we still need it to + // compare with other declarations + DeclNameToDeclarations[declName].push_back(recordDecl); + } + } else if (auto *decl = Result.Nodes.getNodeAs("friend_decl")) { + // record types used in friend delarations are not marked referenced in AST + // NOTE: we only consider friend of record type but functions. + // Although function can contain parameters that reference the record type + // and it could be potentially in wrong namespace and unused, we still + // ignore this case since function declared in headers are usually not used. + if (const TypeSourceInfo *tsi = decl->getFriendType()) { + QualType desugared = tsi->getType().getDesugaredType(*Result.Context); + FriendTypes.insert(desugared.getTypePtr()); + } + } +} + +static bool haveSameNamespace(const CXXRecordDecl *decl1, + const CXXRecordDecl *decl2) { + const DeclContext *parentDecl1 = decl1->getLexicalParent(); + const DeclContext *parentDecl2 = decl2->getLexicalParent(); + // Since we only matched decls whose parent is Namespace or TranslationUnit + // Decl, parent should be either TU or NS + if (parentDecl1->getDeclKind() == Decl::TranslationUnit || + parentDecl2->getDeclKind() == Decl::TranslationUnit) { + return parentDecl1 == parentDecl2; + } + auto *ns1 = NamespaceDecl::castFromDeclContext(parentDecl1); + auto *ns2 = NamespaceDecl::castFromDeclContext(parentDecl2); + return ns1->getOriginalNamespace() == ns2->getOriginalNamespace(); +} + +static std::string getNameOfNamespace(const CXXRecordDecl *decl) { + std::stack stk; + for (auto *parentDecl = decl->getLexicalParent(); + parentDecl->getDeclKind() != Decl::TranslationUnit; + parentDecl = parentDecl->getLexicalParent()) { + auto *namespaceDecl = dyn_cast(parentDecl); + stk.push(namespaceDecl); + } + std::string ns = ""; + while (!stk.empty()) { + auto *namespaceDecl = stk.top(); + stk.pop(); + ns += namespaceDecl->getName().data(); + ns += "::"; + } + if (ns.length() == 0) { + ns = "Top level"; + } + return ns; +} + +void ForwardDeclarationNamespaceCheck::onEndOfTranslationUnit() { + // Iterate each group of declarations grouped by names. + for (auto const &iterator : DeclNameToDeclarations) { + auto &declarations = iterator.second; + // if more than 1 declarations exist, check if all have the same namesapce + for (auto curIter = declarations.begin(); curIter != declarations.end(); + ++curIter) { + const CXXRecordDecl *curDecl = *curIter; + if (curDecl->hasDefinition() || curDecl->isReferenced()) { + continue; // skip forward decl with definition or those referenced + } + if (FriendTypes.find(curDecl->getTypeForDecl()) != FriendTypes.end()) { + continue; // skip forward decl referenced in friend decl + } + if (curDecl->getLocation().isMacroID() || + curDecl->getLocation().isInvalid()) { + continue; // ignore + } + // compare with all other declarations with the same name + for (auto declIter = declarations.begin(); declIter != declarations.end(); + ++declIter) { + if (declIter == curIter) { + continue; // don't compare with self + } + auto decl = *declIter; + if (!curDecl->hasDefinition() && !haveSameNamespace(curDecl, decl)) { + diag(curDecl->getLocation(), + "declaration is never referenced, but a declaration with the " + "same name '%0' found in another namespace '%1'") + << curDecl->getName() << getNameOfNamespace(decl); + diag(decl->getLocation(), "a declaration of '%0' is found here", + DiagnosticIDs::Note) + << decl->getName(); + break; // FIXME: if want location of all other decls, don't break + } + } + // check if a definition in another namespace exists + auto declName = curDecl->getNameAsString(); + if (DeclNameToDefinitions.find(declName) == DeclNameToDefinitions.end()) { + continue; // No definition in this translation unit, skip it + } + // make a warning for each definition with the same name but different ns + auto &definitions = DeclNameToDefinitions[declName]; + for (auto def : definitions) { + diag(curDecl->getLocation(), + "no definition found for '%0', but a definition with " + "the same name '%1' found in another namespace '%2'") + << curDecl->getName() << def->getName() << getNameOfNamespace(def); + diag(def->getLocation(), "a definition of '%0' is found here", + DiagnosticIDs::Note) + << def->getName(); + } + } + } +} + +} // namespace misc +} // namespace tidy +} // namespace clang Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -35,6 +35,8 @@ #include "UnusedParametersCheck.h" #include "UnusedRAIICheck.h" #include "VirtualNearMissCheck.h" +#include "ForwardDeclarationNamespaceCheck.h" + namespace clang { namespace tidy { @@ -90,6 +92,8 @@ CheckFactories.registerCheck("misc-unused-raii"); CheckFactories.registerCheck( "misc-virtual-near-miss"); + CheckFactories.registerCheck( + "misc-forward-declaration-namespace"); } }; Index: test/clang-tidy/misc-forward-declaration-namespace.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-forward-declaration-namespace.cpp @@ -0,0 +1,154 @@ +// RUN: %check_clang_tidy %s misc-forward-declaration-namespace %t + +namespace { +// Delaration in the wrong namespace +class T_A; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'T_A' found in another namespace 'na::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'T_A' is found here +// CHECK-MESSAGES: :[[@LINE-3]]:7: warning: no definition found for 'T_A', but a definition with the same name 'T_A' found in another namespace 'Top level' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a definition of 'T_A' is found here +} + +namespace na { +// Delaration in the wrong namespace +class T_A; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'T_A' found in another namespace '::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'T_A' is found here +// CHECK-MESSAGES: :[[@LINE-3]]:7: warning: no definition found for 'T_A', but a definition with the same name 'T_A' found in another namespace 'Top level' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a definition of 'T_A' is found here +} + +class T_A; + +class T_A { + int x; +}; + + +namespace na { +class T_B; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'T_B' found in another namespace 'nb::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'T_B' is found here +// CHECK-MESSAGES: :[[@LINE-3]]:7: warning: no definition found for 'T_B', but a definition with the same name 'T_B' found in another namespace 'nb::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a definition of 'T_B' is found here +} + +namespace nb { +class T_B; +} + +namespace nb { +class T_B { + int x; +}; +} + +namespace na { +class T_B; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'T_B' found in another namespace 'nb::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'T_B' is found here +// CHECK-MESSAGES: :[[@LINE-3]]:7: warning: no definition found for 'T_B', but a definition with the same name 'T_B' found in another namespace 'nb::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a definition of 'T_B' is found here +} + +// simple forward declaration. although never used, but no same declaration +// found in other namespace +class OUTSIDER; + +namespace na { +// Referenced declaration, no warning +class OUTSIDER_1; +} + +void f(na::OUTSIDER_1); + +namespace nc { +// referenced as friend +class OUTSIDER_1; + +class OOP { + friend struct OUTSIDER_1; +}; +} + +namespace nd { +class OUTSIDER_1; +void f(OUTSIDER_1 *); +} + +namespace nb { +class OUTSIDER_1; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'OUTSIDER_1' found in another namespace 'na::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'OUTSIDER_1' is found here +} + + +namespace na { +template +class T_C; +} + +namespace nb { +// FIXME: this is an error, but we don't consider template class declaration now +template +class T_C; +} + +namespace na { +template +class T_C { + int x; +}; +} + +namespace na { + +template + class T_TEMP { + template + struct rebind + { typedef T_TEMP<_Tp1> other; }; +}; + +// Ignoring class template specialization +template class T_TEMP; +} + +namespace nb { + +template + class T_TEMP_1 { + template + struct rebind + { typedef T_TEMP_1<_Tp1> other; }; +}; + +// Ignoring class template specialization +extern template class T_TEMP_1; +} + +namespace nd { +class D; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration is never referenced, but a declaration with the same name 'D' found in another namespace 'nd::ne::' [misc-forward-declaration-namespace] +// CHECK-MESSAGES: note: a declaration of 'D' is found here +} + +namespace nd { +namespace ne { +class D; +} +} + +int f(nd::ne::D &d); + + +// Check if we can ignore this case +template +class Observer { + class Impl; +}; + +template +class Observer::Impl { + +};