llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) <details> <summary>Changes</summary> #<!-- -->169166 reported some false positives in this check. They stemmed from the fact that it assumed `TypedefTypeLoc`s and `TagTypeLoc`s couldn't be dependent. Turns out, this incorrect assumption leads to some false *negatives* as well: https://godbolt.org/z/K6EvfrE6a. This PR fixes those. (I restructured the code a bit because the "straightforward" fix would have introduced a bit too much duplication). --- Full diff: https://github.com/llvm/llvm-project/pull/171947.diff 2 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp (+30-34) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp (+19-3) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp index 5f2519ce9d5c3..0816625b1937d 100644 --- a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp @@ -18,9 +18,9 @@ using namespace clang::ast_matchers; namespace clang::tidy::readability { void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(typeLoc(unless(hasAncestor(decl(isInstantiated())))) - .bind("nonDependentTypeLoc"), - this); + Finder->addMatcher( + typeLoc(unless(hasAncestor(decl(isInstantiated())))).bind("typeLoc"), + this); if (!getLangOpts().CPlusPlus20) return; @@ -44,38 +44,34 @@ void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) { } void RedundantTypenameCheck::check(const MatchFinder::MatchResult &Result) { + const TypeLoc TL = [&] { + if (const auto *TL = Result.Nodes.getNodeAs<TypeLoc>("typeLoc")) + return TL->getType()->isDependentType() ? TypeLoc() : *TL; + + auto TL = *Result.Nodes.getNodeAs<TypeLoc>("dependentTypeLoc"); + while (const TypeLoc Next = TL.getNextTypeLoc()) + TL = Next; + return TL; + }(); + + if (TL.isNull()) + return; + const SourceLocation ElaboratedKeywordLoc = [&] { - if (const auto *NonDependentTypeLoc = - Result.Nodes.getNodeAs<TypeLoc>("nonDependentTypeLoc")) { - if (NonDependentTypeLoc->getType()->isDependentType()) - return SourceLocation(); - - if (const auto TL = NonDependentTypeLoc->getAs<TypedefTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - - if (const auto TL = NonDependentTypeLoc->getAs<TagTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - - if (const auto TL = NonDependentTypeLoc - ->getAs<DeducedTemplateSpecializationTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - - if (const auto TL = - NonDependentTypeLoc->getAs<TemplateSpecializationTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - } else { - TypeLoc InnermostTypeLoc = - *Result.Nodes.getNodeAs<TypeLoc>("dependentTypeLoc"); - while (const TypeLoc Next = InnermostTypeLoc.getNextTypeLoc()) - InnermostTypeLoc = Next; - - if (const auto TL = InnermostTypeLoc.getAs<DependentNameTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - - if (const auto TL = - InnermostTypeLoc.getAs<TemplateSpecializationTypeLoc>()) - return TL.getElaboratedKeywordLoc(); - } + if (const auto CastTL = TL.getAs<TypedefTypeLoc>()) + return CastTL.getElaboratedKeywordLoc(); + + if (const auto CastTL = TL.getAs<TagTypeLoc>()) + return CastTL.getElaboratedKeywordLoc(); + + if (const auto CastTL = TL.getAs<DeducedTemplateSpecializationTypeLoc>()) + return CastTL.getElaboratedKeywordLoc(); + + if (const auto CastTL = TL.getAs<TemplateSpecializationTypeLoc>()) + return CastTL.getElaboratedKeywordLoc(); + + if (const auto CastTL = TL.getAs<DependentNameTypeLoc>()) + return CastTL.getElaboratedKeywordLoc(); return SourceLocation(); }(); diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp index e8fcd9bcd5731..96bd7b6412724 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp @@ -271,13 +271,21 @@ WHOLE_DECLARATION_IN_MACRO; template <typename T> struct Wrapper {}; template <typename T> struct ClassWrapper { - using R = T; - Wrapper<R> f(); + using R = T; + Wrapper<R> f(); + R g(); }; template <typename T> Wrapper<typename ClassWrapper<T>::R> ClassWrapper<T>::f() { - return {}; + return {}; +} + +template <typename T> +typename ClassWrapper<T>::R ClassWrapper<T>::g() { +// CHECK-MESSAGES-20: :[[@LINE-1]]:1: warning: redundant 'typename' [readability-redundant-typename] +// CHECK-FIXES-20: ClassWrapper<T>::R ClassWrapper<T>::g() { + return {}; } template <typename T> struct StructWrapper {}; @@ -285,9 +293,17 @@ template <typename T> class ClassWithNestedStruct { struct Nested {}; StructWrapper<Nested> f(); + Nested g(); }; template <typename T> StructWrapper<typename ClassWithNestedStruct<T>::Nested> ClassWithNestedStruct<T>::f() { return {}; } + +template <typename T> +typename ClassWithNestedStruct<T>::Nested ClassWithNestedStruct<T>::g() { +// CHECK-MESSAGES-20: :[[@LINE-1]]:1: warning: redundant 'typename' [readability-redundant-typename] +// CHECK-FIXES-20: ClassWithNestedStruct<T>::Nested ClassWithNestedStruct<T>::g() { + return {}; +} `````````` </details> https://github.com/llvm/llvm-project/pull/171947 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
