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

[alexfh]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

22 ↗

(On Diff #57778)

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 ↗

(On Diff #57778)

Debug output?

102 ↗

(On Diff #57778)

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

clang-tidy/misc/UnusedUsingDeclsCheck.h

39 ↗

(On Diff #57778)

SmallPtrSet should be more efficient here.

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

86 ↗

(On Diff #57778)

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 ↗

(On Diff #57778)

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 ↗

(On Diff #57785)

Will do it in a follow-up.

[alexfh]

LG with a couple of nits.

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

22 ↗

(On Diff #57790)

ShouldCheckDecl might convey the idea better.

59 ↗

(On Diff #57790)

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 ↗

(On Diff #57790)

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

[alexfh]

clang-tidy/misc/UnusedUsingDeclsCheck.cpp

59 ↗

(On Diff #57790)

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 ↗

(On Diff #57790)

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

clang-tools-extra/trunk/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-tools-extra/trunk/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

clang-tools-extra/trunk/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