[clang-tidy] Fix readability-redundant-smartptr-get for MSVC STL

[fgross]

Summary

The MSVC STL defines smart pointer operator* and operator-> as method templates. The existing duck typing implementation doesn't catch this implementation. To keep the duck typing simple, I added the already existing list of known standard to the matchers in registerMatchersForGetArrowStart.

[lebedev.ri]

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

60 ↗

(On Diff #196898)

not clang-formatted

[fgross]

Fixed format

[Eugene.Zelenko]

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

96 ↗

(On Diff #196911)

Please add new line.

[Eugene.Zelenko]

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

37 ↗

(On Diff #196911)

Unnecessary empty line.

[fgross]

Fixed test file format

[aaron.ballman]

LGTM aside from a commenting nit.

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

46 ↗

(On Diff #196960)

Missing a full stop at the end of the comment.

clang-tidy/readability/RedundantSmartptrGetCheck.cpp

return cxxMemberCallExpr(
                     .bind("smart_pointer")),
              unless(callee(memberExpr(hasObjectExpression(cxxThisExpr())))),
              callee(cxxMethodDecl(
                  hasName("get"),
                  returns(qualType(pointsTo(type().bind("getType")))))))
       .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(QuacksLikeASmartptr)))),
-                     Callback);
+  Finder->addMatcher(
+      memberExpr(expr().bind("memberExpr"), isArrow(),
+                 hasObjectExpression(ignoringImpCasts(callToGet(Smartptr)))),
 
   // Catch '*ptr.get()' or '*ptr->get()'
   Finder->addMatcher(
-      unaryOperator(hasOperatorName("*"),
-                    hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
+      unaryOperator(hasOperatorName("*"), hasUnaryOperand(callToGet(Smartptr))),
       Callback);
 
   // Catch '!ptr.get()'
-  const auto CallToGetAsBool = ignoringParenImpCasts(callToGet(recordDecl(
-      QuacksLikeASmartptr, has(cxxConversionDecl(returns(booleanType()))))));
+  const auto CallToGetAsBool = ignoringParenImpCasts(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);
 }
 
 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 a list of known standard types.
-
-  const auto IsAKnownSmartptr =
-      recordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
+  // For now, lets keep it to the known standard types.
 
   // Matches against nullptr.
   Finder->addMatcher(
       binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
                      hasEitherOperand(ignoringImpCasts(
                          anyOf(cxxNullPtrLiteralExpr(), gnuNullExpr(),
                                integerLiteral(equals(0))))),
-                     hasEitherOperand(callToGet(IsAKnownSmartptr))),
+                     hasEitherOperand(callToGet(knownSmartptr()))),
       Callback);
 
   // FIXME: Match and fix if (l.get() == r.get()).
 }
 
 } // namespace
 
 void RedundantSmartptrGetCheck::storeOptions(

test/clang-tidy/readability-redundant-smartptr-get-msvc.cpp

+// RUN: %check_clang_tidy %s readability-redundant-smartptr-get %t
+
+#define NULL __null
+
+namespace std {
+
+// MSVC headers define operator templates instead of plain operators.
+
+template <typename T>
+struct unique_ptr {
+  template <typename T2 = T>
+  T2& operator*() const;
+  template <typename T2 = T>
+  T2* operator->() const;
+  T* get() const;
+  explicit operator bool() const noexcept;
+};
+
+template <typename T>
+struct shared_ptr {
+  template <typename T2 = T>
+  T2& operator*() const;
+  template <typename T2 = T>
+  T2* operator->() const;
+  T* get() const;
+  explicit operator bool() const noexcept;
+};
+
+}  // namespace std
+
+struct Bar {
+  void Do();
+  void ConstDo() const;
+};
+
+void Positive() {
+  std::unique_ptr<Bar>* up;
+  (*up->get()).Do();
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant get() call
+  // CHECK-MESSAGES: (*up->get()).Do();
+  // CHECK-FIXES: (**up).Do();
+
+  std::unique_ptr<int> uu;
+  std::shared_ptr<double> *ss;
+  bool bb = uu.get() == nullptr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: redundant get() call
+  // CHECK-MESSAGES: uu.get() == nullptr;
+  // CHECK-FIXES: bool bb = uu == nullptr;
+
+  if (up->get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (up->get());
+  // CHECK-FIXES: if (*up);
+  if ((uu.get()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: if ((uu.get()));
+  // CHECK-FIXES: if ((uu));
+  bb = !ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: redundant get() call
+  // CHECK-MESSAGES: bb = !ss->get();
+  // CHECK-FIXES: bb = !*ss;
+
+  bb = nullptr != ss->get();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
+  // CHECK-MESSAGES: nullptr != ss->get();
+  // CHECK-FIXES: bb = nullptr != *ss;
+
+  bb = std::unique_ptr<int>().get() == NULL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = std::unique_ptr<int>().get() == NULL;
+  // CHECK-FIXES: bb = std::unique_ptr<int>() == NULL;
+  bb = ss->get() == NULL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
+  // CHECK-MESSAGES: bb = ss->get() == NULL;
+  // CHECK-FIXES: bb = *ss == NULL;
+
+  std::unique_ptr<int> x, y;
+  if (x.get() == nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (x.get() == nullptr);
+  // CHECK-FIXES: if (x == nullptr);
+  if (nullptr == y.get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: redundant get() call
+  // CHECK-MESSAGES: if (nullptr == y.get());
+  // CHECK-FIXES: if (nullptr == y);
+  if (x.get() == NULL);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // 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);
+}