From 67853ac4e01a60e9be3db0c1745c7bdc58dfa446 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Tue, 3 Sep 2019 14:13:00 +0000 Subject: [PATCH] [clang-tidy] Fix a false positive in unused-using-decl check The previous matcher "hasAnyTemplateArgument(templateArgument())" only matches the first template argument, but the check wants to iterate all template arguments. This patch fixes this. Also some refactorings in this patch (to make the code reusable). llvm-svn: 370760 --- .../clang-tidy/misc/UnusedUsingDeclsCheck.cpp | 77 +++++++++++++--------- .../test/clang-tidy/misc-unused-using-decls.cpp | 9 +++ 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp b/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp index 256c676..dafd32e 100644 --- a/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp @@ -17,6 +17,29 @@ namespace clang { namespace tidy { namespace misc { +namespace { +// FIXME: Move ASTMatcher library. +AST_POLYMORPHIC_MATCHER_P( + forEachTemplateArgument, + AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl, + TemplateSpecializationType, FunctionDecl), + clang::ast_matchers::internal::Matcher, InnerMatcher) { + ArrayRef TemplateArgs = + clang::ast_matchers::internal::getTemplateSpecializationArgs(Node); + clang::ast_matchers::internal::BoundNodesTreeBuilder Result; + bool Matched = false; + for (const auto &Arg : TemplateArgs) { + clang::ast_matchers::internal::BoundNodesTreeBuilder ArgBuilder(*Builder); + if (InnerMatcher.matches(Arg, Finder, &ArgBuilder)) { + Matched = true; + Result.addMatch(ArgBuilder); + } + } + *Builder = std::move(Result); + return Matched; +} +} // namespace + // A function that helps to tell whether a TargetDecl in a UsingDecl will be // checked. Only variable, function, function template, class template, class, // enum declaration and enum constant declaration are considered. @@ -37,11 +60,10 @@ void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher(callExpr(callee(unresolvedLookupExpr().bind("used"))), this); Finder->addMatcher( - callExpr(hasDeclaration(functionDecl(hasAnyTemplateArgument( - anyOf(refersToTemplate(templateName().bind("used")), - refersToDeclaration(functionDecl().bind("used"))))))), + callExpr(hasDeclaration(functionDecl( + forEachTemplateArgument(templateArgument().bind("used"))))), this); - Finder->addMatcher(loc(templateSpecializationType(hasAnyTemplateArgument( + Finder->addMatcher(loc(templateSpecializationType(forEachTemplateArgument( templateArgument().bind("used")))), this); } @@ -80,52 +102,47 @@ void UnusedUsingDeclsCheck::check(const MatchFinder::MatchResult &Result) { 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 *Used = Result.Nodes.getNodeAs("used")) { + + // Mark a corresponding using declaration as used. + auto RemoveNamedDecl = [&](const NamedDecl *Used) { + removeFromFoundDecls(Used); + // Also remove variants of Used. if (const auto *FD = dyn_cast(Used)) { removeFromFoundDecls(FD->getPrimaryTemplate()); } else if (const auto *Specialization = dyn_cast(Used)) { - Used = Specialization->getSpecializedTemplate(); + removeFromFoundDecls(Specialization->getSpecializedTemplate()); + } else if (const auto *FD = dyn_cast(Used)) { + if (const auto *FDT = FD->getPrimaryTemplate()) + removeFromFoundDecls(FDT); + } else if (const auto *ECD = dyn_cast(Used)) { + if (const auto *ET = ECD->getType()->getAs()) + removeFromFoundDecls(ET->getDecl()); } - removeFromFoundDecls(Used); + }; + // We rely on the fact that the clang AST is walked in order, usages are only + // marked after a corresponding using decl has been found. + if (const auto *Used = Result.Nodes.getNodeAs("used")) { + RemoveNamedDecl(Used); return; } if (const auto *Used = Result.Nodes.getNodeAs("used")) { - // FIXME: Support non-type template parameters. if (Used->getKind() == TemplateArgument::Template) { if (const auto *TD = Used->getAsTemplate().getAsTemplateDecl()) removeFromFoundDecls(TD); } else if (Used->getKind() == TemplateArgument::Type) { if (auto *RD = Used->getAsType()->getAsCXXRecordDecl()) removeFromFoundDecls(RD); + } else if (Used->getKind() == TemplateArgument::Declaration) { + RemoveNamedDecl(Used->getAsDecl()); } return; } - if (const auto *Used = Result.Nodes.getNodeAs("used")) { - removeFromFoundDecls(Used->getAsTemplateDecl()); - return; - } - if (const auto *DRE = Result.Nodes.getNodeAs("used")) { - if (const auto *FD = dyn_cast(DRE->getDecl())) { - if (const auto *FDT = FD->getPrimaryTemplate()) - removeFromFoundDecls(FDT); - else - removeFromFoundDecls(FD); - } else if (const auto *VD = dyn_cast(DRE->getDecl())) { - removeFromFoundDecls(VD); - } else if (const auto *ECD = dyn_cast(DRE->getDecl())) { - removeFromFoundDecls(ECD); - if (const auto *ET = ECD->getType()->getAs()) - removeFromFoundDecls(ET->getDecl()); - } + RemoveNamedDecl(DRE->getDecl()); + return; } // Check the uninstantiated template function usage. if (const auto *ULE = Result.Nodes.getNodeAs("used")) { diff --git a/clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp b/clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp index eed0bae..9511160 100644 --- a/clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp +++ b/clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp @@ -31,6 +31,8 @@ class N {}; template class P {}; const int Constant = 0; +template class Q {}; + class Base { public: void f(); @@ -169,6 +171,8 @@ using n::N; using n::Constant; // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: using decl 'Constant' is unused +using n::Q; + // ----- Usages ----- void f(B b); void g() { @@ -202,3 +206,8 @@ template void h(n::M* t); template void i(n::P* t) {} template void i(n::P* t); + +template class U> class Bar {}; +// We used to report Q unsued, because we only checked the first template +// argument. +Bar *bar; -- 2.7.4