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

[ioeric]

Summary

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.

[akuegel]

Just a couple of nits in the first round.

clang-tidy/misc/CMakeLists.txt

30 ↗

(On Diff #47792)

Put this into alphabetical position in this list.

clang-tidy/misc/MiscTidyModule.cpp

38 ↗

(On Diff #47792)

Nit: put this in alphabetical position.

39 ↗

(On Diff #47792)

Is this additional empty line intentional?

95 ↗

(On Diff #47792)

Again, please put into alphabetical order.

[ioeric]

• formatted code

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[akuegel]

In general, please make sure all comments you write are a complete phrase, don't abbreviate. See also http://llvm.org/docs/CodingStandards.html#commenting

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

46 ↗

(On Diff #47796)

Seems there is an unnecessary space between "used" and "in".

62 ↗

(On Diff #47796)

According to http://llvm.org/docs/CodingStandards.html#assert-liberally
I think you should actually assert that this is a friend_decl, since you only expect to match either record_decls or friend_decls.

64 ↗

(On Diff #47796)

I don't really understand that NOTE; can you please make it clearer?

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

10 ↗

(On Diff #47796)

Please use the header guard
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H_

22 ↗

(On Diff #47796)

It would be nice if you can add a comment here describing what your check does.
See for example the IncorrectRoundings check.

42 ↗

(On Diff #47796)

Don't forget to update the comment here accordingly.

[hokein]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

53 ↗

(On Diff #47796)

Please following LLVM code style (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). The variable name should be in camel case.

76 ↗

(On Diff #47796)

Instead of using static function here, I prefer to define these function in an anonymous namespace.

Looks like the function does more its name says, it also checks whether it's in the same TranslationUnit?

90 ↗

(On Diff #47796)

The same, put it in anonymous namespace.

116 ↗

(On Diff #47796)

You can use for-range loop.

127 ↗

(On Diff #47796)

The comment seems useless.

130 ↗

(On Diff #47796)

for-range loop.

154 ↗

(On Diff #47796)

Use const auto *def : definitions to make the type more clear.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

22 ↗

(On Diff #47796)

Normally, for each check, you need to add rst document in docs/clang-tidy/checks/. The add_new_check.py will help you to do that.

[ioeric]

• improved code style and commenting; added asserts for assumptions; added ast doc

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[alexfh]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

77 ↗

(On Diff #47968)

Not really, it's actually more common in LLVM code to use static functions instead of putting them in unnamed namespaces: http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

1 ↗

(On Diff #47796)

nit: Remove extra minuses to align this to 80 columns.

10 ↗

(On Diff #47796)

This should be LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H_

[ioeric]

• improved formatting; changed back to static function to follow llvm coding standard

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[hokein]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

43 ↗

(On Diff #47972)

s/declaration/declarations/

159 ↗

(On Diff #47972)

s/definitions/Definitions

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

37 ↗

(On Diff #47972)

Adding the following paragraph at the end of the comment.

// For the user-facing documentation see:
// http://clang.llvm.org/extra/clang-tidy/checks/misc-forward-declaration-namespace.html

38 ↗

(On Diff #47972)

Extra blank here.

49 ↗

(On Diff #47972)

I think you can use llvm::StringRef as map key? CXXRecordDecl->getName() returns llvm::StringRef.

Using string here will require extra std::string construction&copy operations.

10 ↗

(On Diff #47796)

I think it should be LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_FORWARDDECLARATIONNAMESPACECHECK_H, no underscore at the end.

docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

1 ↗

(On Diff #47972)

You also need to add an entry item in docs/clang-tidy/checks/list.rst

6 ↗

(On Diff #47972)

Extra blank line here.

[ioeric]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

49 ↗

(On Diff #47972)

I didn't know StringRef works with std::map insce it stores a pointer to a c string and the length of the string. But looks like there is a StringMap that can be used with StringRef, I'll look into this.

docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

6 ↗

(On Diff #47972)

I saw there are two blank lines put here in many other .rst files. Is this a convention?

[ioeric]

• added check to list.rst; using StringRef as map key to improve effiency

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[ioeric]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

50 ↗

(On Diff #47977)

You are right! llvm::StringRef should work fine with std::map since the related compare operations are provided.

[ioeric]

• using llvm::StringMap instead of std::map since it is optimized for string key

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[akuegel]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

81 ↗

(On Diff #47982)

Please align with previous line.

125 ↗

(On Diff #47982)

nit: namesapce -> namespace.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

11 ↗

(On Diff #47982)

True, I just looked at other headers and you are right. Didn't find anything in LLVM styleguide about this though, but maybe I overlooked it.

docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

20 ↗

(On Diff #47982)

nit: "warning" -> "warnings", "fix" -> "a fix".

[ioeric]

• removed redundant headers and did more formatting

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[ioeric]

• removed unused headers

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[akuegel]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

56 ↗

(On Diff #48173)

nit: add a space between "namespace" and "(".

71 ↗

(On Diff #48173)

nit: start variable name with uppercase letter.

72 ↗

(On Diff #48173)

nit: start variable name with uppercase letter.

158 ↗

(On Diff #48173)

nit: add a space between "name" and "(".

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

58 ↗

(On Diff #48173)

Please also remove the trailing "_".

test/clang-tidy/misc-forward-declaration-namespace.cpp

107 ↗

(On Diff #48173)

The formatting looks weird. It is a test, but still I think I would prefer the normal formatting.

120 ↗

(On Diff #48173)

Same here about formatting.

[ioeric]

• formatting and code styling

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[akuegel]

LGTM
(you need someone else to approve this change though).

[hokein]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

64 ↗

(On Diff #48177)

s/classes/Classes
Please make sure all the comments is a complete sentence, use proper capitalization, punctuation.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

21 ↗

(On Diff #48177)

missing . at the end of sentence.

test/clang-tidy/misc-forward-declaration-namespace.cpp

4 ↗

(On Diff #48177)

s/Delaration/Declaration

8 ↗

(On Diff #48177)

Top level means global namespace here? If so, why not using ... found in global namespace?

13 ↗

(On Diff #48177)

The same.

15 ↗

(On Diff #48177)

:: means anonymous namespace here.

So changing warning messages to found in <namespace-name> namespace is a more clear way, such as:

found in anonymous namespace
found in global namespace
found in 'na' namespace
found in 'nb::na' namespace

[alexfh]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

30 ↗

(On Diff #48177)

That will filter out all nested class declarations, which is probably fine, but the comment is somewhat confusing.

44 ↗

(On Diff #48177)

Please add a trailing period.

55 ↗

(On Diff #48177)

Please use proper Capitalization and punctuation.

70 ↗

(On Diff #48177)

Please use proper Capitalization and punctuation.

70 ↗

(On Diff #48177)

Enclose inline code snippets in backquotes.

84 ↗

(On Diff #48177)

Please use proper punctuation.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

24 ↗

(On Diff #48177)

I'm not a native speaker, but "existing" doesn't parse in this phrase ("there is ... existing" sounds weird).

25 ↗

(On Diff #48177)

s/is in a potentially wrong namespace/is potentially in a wrong namespace/

35 ↗

(On Diff #48177)

"a fix" or "fixes"

Also, to remove redundant text: "This check doesn't suggest fixes at this point."

46 ↗

(On Diff #48177)

There should be an extra empty line before private. clang-format should take care of this.

51 ↗

(On Diff #48177)

The problem with std::map<llvm::StringRef, ...> is that a StringRef can be created from a temporary std::string or otherwise point to a string which lifetime is less than the lifetime of the map. llvm::StringMap<> is the right thing here.

test/clang-tidy/misc-forward-declaration-namespace.cpp

4 ↗

(On Diff #48177)

Trailing period.

8 ↗

(On Diff #48177)

We usually specify the whole message in one CHECK line and remove repeated parts (like the name of the check) in all other CHECK lines to make the tests easier to read.

[ioeric]

• Formatted comments; changed namespace print format; remove repeated part

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[alexfh]

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

29 ↗

(On Diff #48656)

nit: Please add 3 spaces before the first word to align it with the first word of the previous line. Same for the next line.

30 ↗

(On Diff #48656)

nit: Please use backquotes for inline code snippets.

50 ↗

(On Diff #48656)

Please use const auto * to make it clear that the pointer points to a constant.

64 ↗

(On Diff #48656)

nit: I'd insert an empty line before the comment to make it obvious that it relates to the following lines, not the lines above it.

Same elsewhere.

66 ↗

(On Diff #48656)

The last line doesn't parse well. Maybe "reduce false positive rate."?

71 ↗

(On Diff #48656)

I meant backquotes, not double quotes:

// `A` will  not be marked as "referenced" in the AST.

100 ↗

(On Diff #48656)

Doesn't Decl->getLexicalParent()->printQualifiedName(...) give a reasonable result here?

101 ↗

(On Diff #48656)

Heap allocations should be avoided when possible. Using llvm::SmallVector<> is one way to do this here.

101 ↗

(On Diff #48656)

nit: s/Stk/Stack/

LLVM code normally uses short names, but here saving two characters at the expense of making the name cryptic is a bad choice, IMO.

102 ↗

(On Diff #48656)

nit: const auto *

105 ↗

(On Diff #48656)

const auto *

106 ↗

(On Diff #48656)

1. What ensures ParentDecl is indeed a NamespaceDecl? Please add a comment.

2. If you're sure this cast should always work, use llvm::cast<> instead, it asserts that the type is correct.

113 ↗

(On Diff #48656)

I suppose, it won't make much difference here, but it's more common to use llvm::raw_string_ostream for this purpose in LLVM.

114 ↗

(On Diff #48656)

nit: No need for the parentheses around the condition.

122 ↗

(On Diff #48656)

nit: It's not an Iterator, it's a KeyValue or something like that.

123 ↗

(On Diff #48656)

const auto &

138 ↗

(On Diff #48656)

const auto *

145 ↗

(On Diff #48656)

I'd move the actual name closer to the start of the message:

"declaration '%0' is never referenced, but a declaration with the "
"same name is found in another namespace '%1'"

Also, warning messages are not complete sentences, so they don't need capitalization and a trailing period. Same for notes.

151 ↗

(On Diff #48656)

nit: "We only generate" and "for each declaration."

160 ↗

(On Diff #48656)

nit: Trailing period missing.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

35 ↗

(On Diff #48656)

nit: "warnings,"

50 ↗

(On Diff #48656)

Use llvm::SmallPtrSet<const Type*, ...>

docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

7 ↗

(On Diff #48656)

Don't think so. Rather a copy-paste from a bad source or something like that.

test/clang-tidy/misc-forward-declaration-namespace.cpp

15 ↗

(On Diff #48656)

I think, the message should use 'global namespace' only when referring to the global namespace, not anything inside it. We also need to distinguish global namespace from namespace global { ... } (same for "anonymous namespace" vs namespace anonymous { ... }).

found in another namespace '<global>'
found in another namespace '::<anonymous namespace>'
found in another namespace '::asdf::<anonymous namespace>'

[ioeric]

• Changed the implementation of getNameOfNamespace to use provided interface; more formatting

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[alexfh]

Looks better now, still a few comments.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

31 ↗

(On Diff #48680)

Single backquotes should be fine. Three backquotes are used for starting a code block.

110 ↗

(On Diff #48680)

getQualifiedNameAsString is deprecated. You should use printQualifiedName instead.

145 ↗

(On Diff #48680)

nit: "We only generates" => "We only generate"

docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

19 ↗

(On Diff #48680)

nit: add a comma after "warnings".

test/clang-tidy/misc-forward-declaration-namespace.cpp

15 ↗

(On Diff #48680)

How is an anonymous namespace inside a named namespace represented in the message? ::na::(anonymous) or (anonymous)? Please add a test.

[ioeric]

• added test case for nested anonymous namespace

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[alexfh]

Looks good with one last comment. Once you address it, I can submit the patch for you.

clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

26 ↗

(On Diff #48903)

1. Three backquotes => one backqoute
2. s/. Eg/, e.g./
3. s/class A inside A/class A inside A/ (with backqoutes)

[ioeric]

• final formatting

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

[alexfh]

The patch doesn't apply cleanly. Please update your working copy and regenerate the patch.

[ioeric]

• Merge remote-tracking branch 'origin/master'

Updating D17195: Added a check for forward declaration in the potentially wrong namespace in clang-tidy

[misc-forward-declaration-namespace]

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DefinitionsInHeadersCheck.cpp
+  ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.h

+//===--- 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

clang-tools-extra/trunk/clang-tidy/misc/ForwardDeclarationNamespaceCheck.cpp

+//===--- 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

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DefinitionsInHeadersCheck.h"
+#include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<AssignOperatorSignatureCheck>(
         "misc-assign-operator-signature");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "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>(
         "misc-incorrect-roundings");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    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
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-forward-declaration-namespace.rst

+.. 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.

clang-tools-extra/trunk/test/clang-tidy/misc-forward-declaration-namespace.cpp

+// 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 {
+};