[clang-tidy] make readability-redundant-smartptr-get report get() usage in conditions

[omtcyfz]

Summary

This patch extends clang-tidy's readability-redundant-smartptr-get to produce warnings for previously unsupported cases:

std::unique_ptr<void> ptr;
if (ptr.get())
if (ptr.get() == NULL)
if (ptr.get() != NULL)

This is intended to fix https://llvm.org/bugs/show_bug.cgi?id=25804, a bug report opened by @Eugene.Zelenko.

However, there still are cases not detected by the check. They can be found in void Negative() function defined in test/clang-tidy/readability-redundant-smartptr-get.cpp.

[alexfh]

LG. Thank you!

[alexfh]

clang-tidy/readability/RedundantSmartptrGetCheck.cpp

64 ↗

(On Diff #72395)

BTW, this can be done using hasAnyName.

[omtcyfz]

Replace anyOf(hasName("..."), hasName("...")) with hasAnyName("...", "...").

[alexfh]

Still LG.

[omtcyfz]

In D24893#551914, @alexfh wrote:

Still LG.

Thank you! Landing the patch.

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

 
 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(
-      anyOf(hasName("::std::unique_ptr"), hasName("::std::shared_ptr")));
+  const auto IsAKnownSmartptr =
+      recordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
 
   // Matches against nullptr.
   Finder->addMatcher(
-      binaryOperator(
-          anyOf(hasOperatorName("=="), hasOperatorName("!=")),
-          hasEitherOperand(ignoringImpCasts(cxxNullPtrLiteralExpr())),
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     hasEitherOperand(ignoringImpCasts(
+                         anyOf(cxxNullPtrLiteralExpr(), gnuNullExpr(),
+                               integerLiteral(equals(0))))),
                      hasEitherOperand(callToGet(IsAKnownSmartptr))),
       Callback);
-  // TODO: Catch ptr.get() == other_ptr.get()
-}
 
+  // Matches against if(ptr.get())
+  Finder->addMatcher(
+      ifStmt(hasCondition(ignoringImpCasts(callToGet(IsAKnownSmartptr)))),
+      Callback);
+
+  // FIXME: Match and fix if (l.get() == r.get()).
+}
 
-}  // namespace
+} // namespace
 
 void RedundantSmartptrGetCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++; the functionality currently does not
   // provide any benefit to other languages, despite being benign.
   if (!getLangOpts().CPlusPlus)
     return;
 
   registerMatchersForGetArrowStart(Finder, this);
bool allReturnTypesMatch(const MatchFinder::MatchResult &Result) {
   const Type *OpArrowType =
       Result.Nodes.getNodeAs<Type>("op->Type")->getUnqualifiedDesugaredType();
   const Type *OpStarType =
       Result.Nodes.getNodeAs<Type>("op*Type")->getUnqualifiedDesugaredType();
   const Type *GetType =
       Result.Nodes.getNodeAs<Type>("getType")->getUnqualifiedDesugaredType();
   return OpArrowType == OpStarType && OpArrowType == GetType;
 }
-}  // namespace
+} // namespace
 
 void RedundantSmartptrGetCheck::check(const MatchFinder::MatchResult &Result) {
-  if (!allReturnTypesMatch(Result)) return;
+  if (!allReturnTypesMatch(Result))
+    return;
 
   bool IsPtrToPtr = Result.Nodes.getNodeAs<Decl>("ptr_to_ptr") != nullptr;
   bool IsMemberExpr = Result.Nodes.getNodeAs<Expr>("memberExpr") != nullptr;
   const Expr *GetCall = Result.Nodes.getNodeAs<Expr>("redundant_get");
   const Expr *Smartptr = Result.Nodes.getNodeAs<Expr>("smart_pointer");
 
   if (IsPtrToPtr && IsMemberExpr) {
     // Ignore this case (eg. Foo->get()->DoSomething());

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

void Positive() {
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: redundant get() call
   // CHECK-MESSAGES: nullptr != ss->get();
   // CHECK-FIXES: bb = nullptr != *ss;
 
   i = *PointerWithOverloadedGet().get();
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: redundant get() call
   // CHECK-MESSAGES: i = *PointerWithOverloadedGet().get();
   // CHECK-FIXES: i = *PointerWithOverloadedGet();
+
+  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);
+  if (x.get());
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant get() call
+  // CHECK-MESSAGES: if (x.get());
+  // CHECK-FIXES: if (x);
 }
 
 void Negative() {
   struct NegPtr {
     int* get();
     int* operator->() {
       return &*this->get();
     }
     int& operator*() {
       return *get();
     }
   };
 
   long l = *PointerWithOverloadedGet().get<long>();
 
   std::unique_ptr<Bar>* u;
   u->get()->Do();
 
   Fail1().get()->Do();
   Fail2().get()->Do();
   const Bar& b = *Fail1().get();
   (*Fail2().get()).Do();
 
   int_ptr ip;
-  bool bb = std::unique_ptr<int>().get() == NULL;
-  bb = ip.get() == nullptr;
-  bb = u->get() == NULL;
+  bool bb = ip.get() == nullptr;
 }