[clang-tools-extra] r261737 - [clang-tidy] Added a check for forward declaration in the potentially wrong namespace

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 24 05:35:33 PST 2016


Author: alexfh
Date: Wed Feb 24 07:35:32 2016
New Revision: 261737

URL: http://llvm.org/viewvc/llvm-project?rev=261737&view=rev
Log:
[clang-tidy] Added a check for forward declaration in the potentially wrong namespace

Adds a new check "misc-forward-declaration-namespace".
In check, A forward declaration is considerred in a potentially wrong namespace
if there is any definition/declaration with the same name exists in a different
namespace.

Reviewers: akuegel, hokein, alexfh

Patch by Eric Liu!

Differential Revision: http://reviews.llvm.org/D17195

Added:
    clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp
    clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h
    clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst
    clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp
Modified:
    clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
    clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
    clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt?rev=261737&r1=261736&r2=261737&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt Wed Feb 24 07:35:32 2016
@@ -6,6 +6,7 @@ add_clang_library(clangTidyMiscModule
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DefinitionsInHeadersCheck.cpp
+  ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp

Added: clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp?rev=261737&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp Wed Feb 24 07:35:32 2016
@@ -0,0 +1,174 @@
+//===--- 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 <stack>
+#include <string>
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ForwardDeclarationNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  // Match all class declarations/definitions *EXCEPT*
+  // 1. implicit classes, e.g. `class A {};` has implicit `class A` inside `A`.
+  // 2. nested classes declared/defined inside another class.
+  // 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 declarations. Classes used in friend declarations are not
+  // marked as referenced in AST. We need to record all record classes used in
+  // friend declarations.
+  Finder->addMatcher(friendDecl().bind("friend_decl"), this);
+}
+
+void ForwardDeclarationNamespaceCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *RecordDecl =
+          Result.Nodes.getNodeAs<CXXRecordDecl>("record_decl")) {
+    StringRef DeclName = RecordDecl->getName();
+    if (RecordDecl->isThisDeclarationADefinition()) {
+      DeclNameToDefinitions[DeclName].push_back(RecordDecl);
+    } else {
+      // If a declaration has no definition, the definition could be in another
+      // namespace (a wrong namespace).
+      // NOTE: even a declaration does have definition, we still need it to
+      // compare with other declarations.
+      DeclNameToDeclarations[DeclName].push_back(RecordDecl);
+    }
+  } else {
+    const auto *Decl = Result.Nodes.getNodeAs<FriendDecl>("friend_decl");
+    assert(Decl && "Decl is neither record_decl nor friend decl!");
+
+    // Classes used in friend delarations are not marked referenced in AST,
+    // so we need to check classes used in friend declarations manually to
+    // reduce the rate of false positive.
+    // For example, in
+    //    \code
+    //      struct A;
+    //      struct B { friend A; };
+    //    \endcode
+    // `A` will not be marked as "referenced" in the AST.
+    if (const TypeSourceInfo *Tsi = Decl->getFriendType()) {
+      QualType Desugared = Tsi->getType().getDesugaredType(*Result.Context);
+      FriendTypes.insert(Desugared.getTypePtr());
+    }
+  }
+}
+
+static bool haveSameNamespaceOrTranslationUnit(const CXXRecordDecl *Decl1,
+                                               const CXXRecordDecl *Decl2) {
+  const DeclContext *ParentDecl1 = Decl1->getLexicalParent();
+  const DeclContext *ParentDecl2 = Decl2->getLexicalParent();
+
+  // Since we only matched declarations whose parent is Namespace or
+  // TranslationUnit declaration, the parent should be either a translation unit
+  // or namespace.
+  if (ParentDecl1->getDeclKind() == Decl::TranslationUnit ||
+      ParentDecl2->getDeclKind() == Decl::TranslationUnit) {
+    return ParentDecl1 == ParentDecl2;
+  }
+  assert(ParentDecl1->getDeclKind() == Decl::Namespace &&
+         "ParentDecl1 declaration must be a namespace");
+  assert(ParentDecl2->getDeclKind() == Decl::Namespace &&
+         "ParentDecl2 declaration must be a namespace");
+  auto *Ns1 = NamespaceDecl::castFromDeclContext(ParentDecl1);
+  auto *Ns2 = NamespaceDecl::castFromDeclContext(ParentDecl2);
+  return Ns1->getOriginalNamespace() == Ns2->getOriginalNamespace();
+}
+
+static std::string getNameOfNamespace(const CXXRecordDecl *Decl) {
+  const auto *ParentDecl = Decl->getLexicalParent();
+  if (ParentDecl->getDeclKind() == Decl::TranslationUnit) {
+    return "(global)";
+  }
+  const auto *NsDecl = cast<NamespaceDecl>(ParentDecl);
+  std::string Ns;
+  llvm::raw_string_ostream OStream(Ns);
+  NsDecl->printQualifiedName(OStream);
+  OStream.flush();
+  return Ns.empty() ? "(global)" : Ns;
+}
+
+void ForwardDeclarationNamespaceCheck::onEndOfTranslationUnit() {
+  // Iterate each group of declarations by name.
+  for (const auto &KeyValuePair : DeclNameToDeclarations) {
+    const auto &Declarations = KeyValuePair.second;
+    // If more than 1 declaration exists, we check if all are in the same
+    // namespace.
+    for (const auto *CurDecl : Declarations) {
+      if (CurDecl->hasDefinition() || CurDecl->isReferenced()) {
+        continue; // Skip forward declarations that are used/referenced.
+      }
+      if (FriendTypes.count(CurDecl->getTypeForDecl()) != 0) {
+        continue; // Skip forward declarations referenced as friend.
+      }
+      if (CurDecl->getLocation().isMacroID() ||
+          CurDecl->getLocation().isInvalid()) {
+        continue;
+      }
+      // Compare with all other declarations with the same name.
+      for (const auto *Decl : Declarations) {
+        if (Decl == CurDecl) {
+          continue; // Don't compare with self.
+        }
+        if (!CurDecl->hasDefinition() &&
+            !haveSameNamespaceOrTranslationUnit(CurDecl, Decl)) {
+          diag(CurDecl->getLocation(),
+               "declaration '%0' is never referenced, but a declaration with "
+               "the same name 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: We only generate one warning for each declaration.
+        }
+      }
+      // Check if a definition in another namespace exists.
+      const auto DeclName = CurDecl->getName();
+      if (DeclNameToDefinitions.find(DeclName) == DeclNameToDefinitions.end()) {
+        continue; // No definition in this translation unit, we can skip it.
+      }
+      // Make a warning for each definition with the same name (in other
+      // namespaces).
+      const auto &Definitions = DeclNameToDefinitions[DeclName];
+      for (const 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

Added: clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h?rev=261737&view=auto
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h (added)
+++ clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h Wed Feb 24 07:35:32 2016
@@ -0,0 +1,59 @@
+//===--- 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 LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H
+
+#include <set>
+#include <vector>
+#include "llvm/ADT/SmallPtrSet.h"
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Checks if an unused forward declaration is in a wrong namespace.
+///
+/// The check inspects all unused forward declarations and checks if there is
+/// any declaration/definition with the same name, which could indicate
+/// that the forward declaration is potentially in a wrong namespace.
+///
+/// \code
+///   namespace na { struct A; }
+///   namespace nb { struct A {} };
+///   nb::A a;
+///   // warning : no definition found for 'A', but a definition with the same
+///   name 'A' found in another namespace 'nb::'
+/// \endcode
+///
+/// This check can only generate warnings, but it can't suggest fixes at this
+/// point.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-forward-declaration-namespace.html
+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:
+  llvm::StringMap<std::vector<const CXXRecordDecl *>> DeclNameToDefinitions;
+  llvm::StringMap<std::vector<const CXXRecordDecl *>> DeclNameToDeclarations;
+  llvm::SmallPtrSet<const Type *, 16> FriendTypes;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H

Modified: clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp?rev=261737&r1=261736&r2=261737&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp Wed Feb 24 07:35:32 2016
@@ -15,6 +15,7 @@
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DefinitionsInHeadersCheck.h"
+#include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
@@ -55,6 +56,8 @@ public:
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
+    CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
+        "misc-forward-declaration-namespace");
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectRoundings>(

Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=261737&r1=261736&r2=261737&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Wed Feb 24 07:35:32 2016
@@ -51,6 +51,7 @@ Clang-Tidy Checks
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
    misc-definitions-in-headers
+   misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm

Added: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst?rev=261737&view=auto
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst (added)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst Wed Feb 24 07:35:32 2016
@@ -0,0 +1,19 @@
+.. title:: clang-tidy - misc-forward-declaration-namespace
+
+misc-forward-declaration-namespace
+==================================
+
+Checks if an unused forward declaration is in a wrong namespace.
+
+The check inspects all unused forward declarations and checks if there is any
+declaration/definition with the same name existing, which could indicate that
+the forward declaration is in a potentially wrong namespace.
+
+.. code:: c++
+  namespace na { struct A; }
+  namespace nb { struct A {}; }
+  nb::A a;
+  // warning : no definition found for 'A', but a definition with the same name
+  // 'A' found in another namespace 'nb::'
+
+This check can only generate warnings, but it can't suggest a fix at this point.

Added: clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp?rev=261737&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp Wed Feb 24 07:35:32 2016
@@ -0,0 +1,163 @@
+// RUN: %check_clang_tidy %s misc-forward-declaration-namespace %t 
+
+namespace {
+// This is a declaration in a wrong namespace.
+class T_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration 'T_A' is never referenced, but a declaration with the same name 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 '(global)' [misc-forward-declaration-namespace]
+// CHECK-MESSAGES: note: a definition of 'T_A' is found here
+}
+
+namespace na {
+// This is a declaration in a wrong namespace.
+class T_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration 'T_A' is never referenced, but a declaration with the same name found in another namespace '(anonymous)'
+// 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 '(global)'
+// CHECK-MESSAGES: note: a definition of 'T_A' is found here
+}
+
+class T_A;
+
+class T_A {
+  int x;
+};
+
+class NESTED;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: no definition found for 'NESTED', but a definition with the same name 'NESTED' found in another namespace '(anonymous namespace)::nq::(anonymous)'
+// CHECK-MESSAGES: note: a definition of 'NESTED' is found here
+
+namespace {
+namespace nq {
+namespace {
+class NESTED {};
+}
+}
+}
+
+namespace na {
+class T_B;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration 'T_B' is never referenced, but a declaration with the same name found in another namespace 'nb'
+// 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'
+// 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 'T_B' is never referenced, but a declaration with the same name found in another namespace 'nb'
+// 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'
+// CHECK-MESSAGES: note: a definition of 'T_B' is found here
+}
+
+// A simple forward declaration. Although it is never used, but no declaration
+// with the same name is found in other namespace.
+class OUTSIDER;
+
+namespace na {
+// This class is referenced declaration, we don't generate warning.
+class OUTSIDER_1;
+}
+
+void f(na::OUTSIDER_1);
+
+namespace nc {
+// This class is referenced as friend in OOP.
+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 'OUTSIDER_1' is never referenced, but a declaration with the same name found in another namespace 'na'
+// CHECK-MESSAGES: note: a declaration of 'OUTSIDER_1' is found here
+}
+
+
+namespace na {
+template<typename T>
+class T_C;
+}
+
+namespace nb {
+// FIXME: this is an error, but we don't consider template class declaration
+// now.
+template<typename T>
+class T_C;
+}
+
+namespace na {
+template<typename T>
+class T_C {
+  int x;
+};
+}
+
+namespace na {
+
+template <typename T>
+class T_TEMP {
+  template <typename _Tp1>
+  struct rebind { typedef T_TEMP<_Tp1> other; };
+};
+
+// We ignore class template specialization.
+template class T_TEMP<char>;
+}
+
+namespace nb {
+
+template <typename T>
+class T_TEMP_1 {
+  template <typename _Tp1>
+  struct rebind { typedef T_TEMP_1<_Tp1> other; };
+};
+
+// We ignore class template specialization.
+extern template class T_TEMP_1<char>;
+}
+
+namespace nd {
+class D;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration 'D' is never referenced, but a declaration with the same name found in another namespace 'nd::ne'
+// CHECK-MESSAGES: note: a declaration of 'D' is found here
+}
+
+namespace nd {
+namespace ne {
+class D;
+}
+}
+
+int f(nd::ne::D &d);
+
+
+// This should be ignored by the check.
+template <typename... Args>
+class Observer {
+  class Impl;
+};
+
+template <typename... Args>
+class Observer<Args...>::Impl {
+};




More information about the cfe-commits mailing list