Handle uninstantiated templates in redundant get check

[njames93]

clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp

21–45

The patch says its simplifying the check, but this doesn't look like its simplifying it. Seems to be extending it to support dependent members.

158

nit: Spell out the type.

161

nit: Elide braces.

[steveire]

Update

[steveire]

clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp

21–45

Changed the description.

clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.h

RedundantSmartptrGetCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context),
         IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  llvm::Optional<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
 
 private:
   const bool IgnoreMacros;
 };
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTSMARTPTRGETCHECK_H

clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp

 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace readability {
 
 namespace {
 internal::Matcher<Expr> callToGet(const internal::Matcher<Decl> &OnClass) {
-  return cxxMemberCallExpr(
+  return expr(
+             anyOf(cxxMemberCallExpr(
                        on(expr(anyOf(hasType(OnClass),
-                           hasType(qualType(
-                               pointsTo(decl(OnClass).bind("ptr_to_ptr"))))))
+                                     hasType(qualType(pointsTo(
+                                         decl(OnClass).bind("ptr_to_ptr"))))))
                               .bind("smart_pointer")),
-             unless(callee(memberExpr(hasObjectExpression(cxxThisExpr())))),
-             callee(cxxMethodDecl(
+                       unless(callee(
+                           memberExpr(hasObjectExpression(cxxThisExpr())))),
+                       callee(cxxMethodDecl(hasName("get"),
+                                            returns(qualType(pointsTo(
+                                                type().bind("getType"))))))),
+                   cxxDependentScopeMemberExpr(
+                       hasMemberName("get"),
+                       hasObjectExpression(
+                           expr(hasType(qualType(hasCanonicalType(
+                                    templateSpecializationType(hasDeclaration(
+                                        classTemplateDecl(has(cxxRecordDecl(
+                                            OnClass,
+                                            hasMethod(cxxMethodDecl(
                                                 hasName("get"),
-                 returns(qualType(pointsTo(type().bind("getType")))))))
+                                                returns(qualType(
+                                                    pointsTo(type().bind(
+                                                        "getType")))))))))))))))
+                               .bind("smart_pointer")))))
       .bind("redundant_get");
 }
 
 internal::Matcher<Decl> knownSmartptr() {
   return recordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
 }
 
 void registerMatchersForGetArrowStart(MatchFinder *Finder,
                                       MatchFinder::MatchCallback *Callback) {
   const auto QuacksLikeASmartptr = recordDecl(
       recordDecl().bind("duck_typing"),
       has(cxxMethodDecl(hasName("operator->"),
                         returns(qualType(pointsTo(type().bind("op->Type")))))),
       has(cxxMethodDecl(hasName("operator*"), returns(qualType(references(
                                                   type().bind("op*Type")))))));
 
   // Make sure we are not missing the known standard types.
   const auto Smartptr = anyOf(knownSmartptr(), QuacksLikeASmartptr);
 
   // Catch 'ptr.get()->Foo()'
-  Finder->addMatcher(
-      memberExpr(expr().bind("memberExpr"), isArrow(),
-                 hasObjectExpression(ignoringImpCasts(callToGet(Smartptr)))),
+  Finder->addMatcher(memberExpr(expr().bind("memberExpr"), isArrow(),
+                                hasObjectExpression(callToGet(Smartptr))),
                      Callback);
 
   // Catch '*ptr.get()' or '*ptr->get()'
   Finder->addMatcher(
       unaryOperator(hasOperatorName("*"), hasUnaryOperand(callToGet(Smartptr))),
       Callback);
 
   // Catch '!ptr.get()'
-  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(
-      recordDecl(Smartptr, has(cxxConversionDecl(returns(booleanType()))))));
+  const auto CallToGetAsBool = callToGet(
+      recordDecl(Smartptr, has(cxxConversionDecl(returns(booleanType())))));
   Finder->addMatcher(
       unaryOperator(hasOperatorName("!"), hasUnaryOperand(CallToGetAsBool)),
       Callback);
 
   // Catch 'if(ptr.get())'
   Finder->addMatcher(ifStmt(hasCondition(CallToGetAsBool)), Callback);
 
   // Catch 'ptr.get() ? X : Y'
   Finder->addMatcher(conditionalOperator(hasCondition(CallToGetAsBool)),
                      Callback);
+
+  Finder->addMatcher(cxxDependentScopeMemberExpr(hasObjectExpression(
+                         callExpr(has(callToGet(Smartptr))).bind("obj"))),
+                     Callback);
 }
 
 void registerMatchersForGetEquals(MatchFinder *Finder,
                                   MatchFinder::MatchCallback *Callback) {
   // This one is harder to do with duck typing.
   // The operator==/!= that we are looking for might be member or non-member,
   // might be on global namespace or found by ADL, might be a template, etc.
   // For now, lets keep it to the known standard types.
 
   // Matches against nullptr.
   Finder->addMatcher(
       binaryOperator(hasAnyOperatorName("==", "!="),
-                     hasOperands(ignoringImpCasts(anyOf(
-                                     cxxNullPtrLiteralExpr(), gnuNullExpr(),
-                                     integerLiteral(equals(0)))),
+                     hasOperands(anyOf(cxxNullPtrLiteralExpr(), gnuNullExpr(),
+                                       integerLiteral(equals(0))),
                                  callToGet(knownSmartptr()))),
       Callback);
 
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
 } // namespace
 
void RedundantSmartptrGetCheck::check(const MatchFinder::MatchResult &Result) {
 
   const auto *Smartptr = Result.Nodes.getNodeAs<Expr>("smart_pointer");
 
   if (IsPtrToPtr && IsMemberExpr) {
     // Ignore this case (eg. Foo->get()->DoSomething());
     return;
   }
 
+  auto SR = GetCall->getSourceRange();
+  // CXXDependentScopeMemberExpr source range does not include parens
+  // Extend the source range of the get call to account for them.
+  if (isa<CXXDependentScopeMemberExpr>(GetCall))
+    SR.setEnd(Lexer::getLocForEndOfToken(SR.getEnd(), 0, *Result.SourceManager,
+                                         getLangOpts())
+                  .getLocWithOffset(1));
+
   StringRef SmartptrText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(Smartptr->getSourceRange()),
       *Result.SourceManager, getLangOpts());
   // Replace foo->get() with *foo, and foo.get() with foo.
   std::string Replacement = Twine(IsPtrToPtr ? "*" : "", SmartptrText).str();
   diag(GetCall->getBeginLoc(), "redundant get() call on smart pointer")
-      << FixItHint::CreateReplacement(GetCall->getSourceRange(), Replacement);
+      << FixItHint::CreateReplacement(SR, Replacement);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/readability-redundant-smartptr-get.cpp

void Positive() {
   // CHECK-MESSAGES: if (x.get() == NULL);
   // CHECK-FIXES: if (x == NULL);
   if (NULL == x.get());
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: redundant get() call
   // CHECK-MESSAGES: if (NULL == x.get());
   // CHECK-FIXES: if (NULL == x);
 }
 
+template <typename T>
+void testTemplate() {
+  T().get()->Do();
+}
+
+template <typename T>
+void testTemplate2() {
+  std::unique_ptr<T> up;
+  up.get()->Do();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant get() call
+  // CHECK-FIXES: up->Do();
+}
+
+void instantiate() {
+  testTemplate<BarPtr>();
+  testTemplate<std::unique_ptr<Bar>>();
+  testTemplate<Fail2>();
+
+  testTemplate2<Bar>();
+}
+
+struct S {
+
+  void foo() {
+    m_up.get()->Do();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant get() call
+    // CHECK-FIXES: m_up->Do();
+    m_bp.get()->Do();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant get() call
+    // CHECK-FIXES: m_bp->Do();
+  }
+
+  std::unique_ptr<Bar> m_up;
+  BarPtr m_bp;
+};
+
 #define MACRO(p) p.get()
 
 void Negative() {
   struct NegPtr {
     int* get();
     int* operator->() {
       return &*this->get();
     }