[clang-tidy] Handle using-decls with more than one shadow decl.

[alexfh]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

22

This method assumes a rather non-trivial definition of "valid". Please add a comment what exactly this method does.

Also, static is preferred to anonymous namespaces for functions in LLVM style (http://llvm.org/docs/CodingStandards.html#anonymous-namespaces):

... make anonymous namespaces as small as possible, and only use them for class declarations. ...

29

Debug output?

103

.count() for sets is as effective, but results in clearer code.

clang-tidy/misc/UnusedUsingDeclsCheck.h

39

SmallPtrSet should be more efficient here.

test/clang-tidy/misc-unused-using-decls.cpp

86

Not for this patch, but it makes sense to remove trailing comments on deleted lines.

[hokein]

Address comments.

[hokein]

Forgot a comments.

[hokein]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

22

I have renamed to IsCheckable, but it doesn't make more sense here. Do you have better suggestion on the function name?

test/clang-tidy/misc-unused-using-decls.cpp

86

Will do it in a follow-up.

[alexfh]

LG with a couple of nits.

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

22

ShouldCheckDecl might convey the idea better.

59

It's not iterator, so It is a confusing name. Something along the lines of Shadow or UsingShadow should be better.

[hokein]

Address more comments.

[hokein]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

59

Actually, the Using->shadows() returns an iterator range, but I'm fine renaming it here.

[alexfh]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

59

Sure, it returns a llvm::iterator_range<shadow_iterator>, which is just a range adaptor for a pair of iterators. It's not a "collection of iterators", it's a "collection, defined by a pair of iterators". If you iterate over it using a range-based for loop, you get whatever is pointed by the iterators, not iterators.

[hokein]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

59

I see it now. Thanks for the explanations :-).

clang-tidy/misc/UnusedUsingDeclsCheck.h

 //===--- UnusedUsingDeclsCheck.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_UNUSED_USING_DECLS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_USING_DECLS_H
 
 #include "../ClangTidy.h"
-#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include <vector>
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 /// Finds unused using declarations.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/misc-unused-using-decls.html
 class UnusedUsingDeclsCheck : public ClangTidyCheck {
 public:
   UnusedUsingDeclsCheck(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:
   void removeFromFoundDecls(const Decl *D);
 
-  llvm::DenseMap<const Decl*, const UsingDecl*> FoundDecls;
-  llvm::DenseMap<const Decl*, CharSourceRange> FoundRanges;
+  struct UsingDeclContext {
+    explicit UsingDeclContext(const UsingDecl *FoundUsingDecl)
+        : FoundUsingDecl(FoundUsingDecl), IsUsed(false) {}
+    llvm::SmallPtrSet<const Decl *, 4> UsingTargetDecls;
+    const UsingDecl *FoundUsingDecl;
+    CharSourceRange UsingDeclRange;
+    bool IsUsed;
+  };
+
+  std::vector<UsingDeclContext> Contexts;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNUSED_USING_DECLS_H

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
+// A function that helps to tell whether a TargetDecl will be checked.
+// We only check a TargetDecl if :
+//   * The corresponding UsingDecl is not defined in macros or in class
+//     definitions.
+//   * Only variable, function and class types are considered.
+static bool ShouldCheckDecl(const Decl *TargetDecl) {
+  // Ignores using-declarations defined in macros.
+  if (TargetDecl->getLocation().isMacroID())
+    return false;
+
+  // Ignores using-declarations defined in class definition.
+  if (isa<CXXRecordDecl>(TargetDecl->getDeclContext()))
+    return false;
+
+  return isa<RecordDecl>(TargetDecl) || isa<ClassTemplateDecl>(TargetDecl) ||
+         isa<FunctionDecl>(TargetDecl) || isa<VarDecl>(TargetDecl) ||
+         isa<FunctionTemplateDecl>(TargetDecl);
+}
+
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(recordType(DeclMatcher)), this);
   Finder->addMatcher(loc(templateSpecializationType(DeclMatcher)), this);
   Finder->addMatcher(declRefExpr().bind("used"), this);
   Finder->addMatcher(callExpr(callee(unresolvedLookupExpr().bind("used"))),
                      this);
 }
 
 void UnusedUsingDeclsCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Using = Result.Nodes.getNodeAs<UsingDecl>("using")) {
-    // FIXME: Implement the correct behavior for using declarations with more
-    // than one shadow.
-    if (Using->shadow_size() != 1)
-      return;
-    const auto *TargetDecl =
-        Using->shadow_begin()->getTargetDecl()->getCanonicalDecl();
-
-    // Ignores using-declarations defined in macros.
-    if (TargetDecl->getLocation().isMacroID())
-      return;
-
-    // Ignores using-declarations defined in class definition.
-    if (isa<CXXRecordDecl>(TargetDecl->getDeclContext()))
-      return;
-
-    if (!isa<RecordDecl>(TargetDecl) && !isa<ClassTemplateDecl>(TargetDecl) &&
-        !isa<FunctionDecl>(TargetDecl) && !isa<VarDecl>(TargetDecl) &&
-        !isa<FunctionTemplateDecl>(TargetDecl))
-      return;
-
-    FoundDecls[TargetDecl] = Using;
-    FoundRanges[TargetDecl] = CharSourceRange::getCharRange(
+    UsingDeclContext Context(Using);
+    Context.UsingDeclRange = CharSourceRange::getCharRange(
         Using->getLocStart(),
         Lexer::findLocationAfterToken(
             Using->getLocEnd(), tok::semi, *Result.SourceManager,
             Result.Context->getLangOpts(),
             /*SkipTrailingWhitespaceAndNewLine=*/true));
+    for (const auto *UsingShadow : Using->shadows()) {
+      const auto *TargetDecl = UsingShadow->getTargetDecl()->getCanonicalDecl();
+      if (ShouldCheckDecl(TargetDecl))
+        Context.UsingTargetDecls.insert(TargetDecl);
+    }
+    if (!Context.UsingTargetDecls.empty())
+      Contexts.push_back(Context);
     return;
   }
 
   // Mark using declarations as used by setting FoundDecls' value to zero. As
   // the AST is walked in order, usages are only marked after a the
   // corresponding using declaration has been found.
   // FIXME: This currently doesn't look at whether the type reference is
   // actually found with the help of the using declaration.
if (const auto *ULE = Result.Nodes.getNodeAs<UnresolvedLookupExpr>("used")) {
     for (const NamedDecl* ND : ULE->decls()) {
       if (const auto *USD = dyn_cast<UsingShadowDecl>(ND))
         removeFromFoundDecls(USD->getTargetDecl()->getCanonicalDecl());
     }
   }
 }
 
 void UnusedUsingDeclsCheck::removeFromFoundDecls(const Decl *D) {
-  auto I = FoundDecls.find(D->getCanonicalDecl());
-  if (I != FoundDecls.end())
-    I->second = nullptr;
+  for (auto &Context : Contexts) {
+    if (Context.UsingTargetDecls.count(D->getCanonicalDecl()) > 0) {
+      Context.IsUsed = true;
+      break;
+    }
+  }
 }
 
 void UnusedUsingDeclsCheck::onEndOfTranslationUnit() {
-  for (const auto &FoundDecl : FoundDecls) {
-    if (FoundDecl.second == nullptr)
-      continue;
-    diag(FoundDecl.second->getLocation(), "using decl %0 is unused")
-        << FoundDecl.second
-        << FixItHint::CreateRemoval(FoundRanges[FoundDecl.first]);
+  for (const auto &Context : Contexts) {
+    if (!Context.IsUsed) {
+      diag(Context.FoundUsingDecl->getLocation(), "using decl %0 is unused")
+          << Context.FoundUsingDecl
+          << FixItHint::CreateRemoval(Context.UsingDeclRange);
+    }
   }
-  FoundDecls.clear();
+  Contexts.clear();
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-unused-using-decls.cpp

 D UsedInstance;
 D UnusedInstance;
 
 int UsedFunc() { return 1; }
 int UnusedFunc() { return 1; }
 template <typename T> int UsedTemplateFunc() { return 1; }
 template <typename T> int UnusedTemplateFunc() { return 1; }
 template <typename T> int UsedInTemplateFunc() { return 1; }
+void OverloadFunc(int);
+void OverloadFunc(double);
 
 class ostream {
 public:
   ostream &operator<<(ostream &(*PF)(ostream &));
 };
 extern ostream cout;
 ostream &endl(ostream &os);
 }

 
 using n::UsedInTemplateFunc;
 using n::J;
 template <typename T> void Callee() {
   J<T> j;
   UsedInTemplateFunc<T>();
 }
 
+using n::OverloadFunc; // OverloadFunc
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: using decl 'OverloadFunc' is unused
+// CHECK-FIXES: {{^}}// OverloadFunc
+
 #define DEFINE_INT(name)        \
   namespace INT {               \
   static const int _##name = 1; \
   }                             \
   using INT::_##name
 DEFINE_INT(test);
 #undef DEFIND_INT