Extend the check to detect patterns like 'ptr.get() == nullptr'

[sbenza]

Summary

Extend the check to detect patterns like 'ptr.get() == nullptr'
It detects == and != when any argument is a ptr.get() and the other is a
nullptr.
Only supports standard smart pointer types std::unique_ptr and std::shared_ptr.
Does not support the case 'ptr.get() == other.get()' yet.

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

There might be a missing test here (s/shard/shared/)

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

Trying again to see whether the mail gets through...

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

And trying again...

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

Yet another try...

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

And again...

[klimek]

clang-tidy/misc/RedundantSmartptrGet.cpp

60

And finally...

[klimek]

Ok, very sorry for having hijacked this thread... It should all work now, and I'll let you be... :)

[sbenza]

Fixed the typo on shared_ptr and added a shared_ptr to the test.

Fixed the -fix.cpp test to actually check the output.
FileCheck can match the CHECK: lines with themselves if you don't remove them from the input.

[sbenza]

Move the checking logic into shell scripts.
The test will now have one only RUN: line calling the appropriate script.

[djasper]

Two minor comments, otherwise looks good!

clang-tidy/misc/RedundantSmartptrGet.cpp

81

Always add a line break after the "if (...)" in LLVM style.

test/clang-tidy/check_clang_tidy_fix.sh

4

I think with this we can get rid of the unit tests entirely. (I will prepare a follow up patch).

Nice!

test/clang-tidy/check_clang_tidy_output.sh

12

Also set --disable_checks=''.

And same in the other wrapper script.

(I think we might also want to change how those flags behave, but until then, this is the right thing to do..)

[djasper]

test/clang-tidy/check_clang_tidy_output.sh

12

-disable-checks that is..

[alexfh]

Another couple of notes, otherwise looks good!

test/clang-tidy/check_clang_tidy_output.sh

11

Why not use static compilation database instead of generating compile_commands.json?

clang-tidy -checks=${CHECK_TO_RUN} ${INPUT_FILE} -- -std=c++11 | FileCheck ${INPUT_FILE}

13

This being on a line of its own without indentation looks confusing. Could you put the whole pipeline on a single line or use indentation here (and put the pipe on the next line)?

clang-tidy ... \
  | FileCheck ...

[sbenza]

Simplify testing scripts.

test/clang-tidy/check_clang_tidy_fix.sh

4

?!?

test/clang-tidy/check_clang_tidy_output.sh

11

Even better. I got rid of that script.

13

Fixed.
Where is sh-format when you need it? =)

[alexfh]

Thanks! Another couple of comments, though.

test/clang-tidy/check_clang_tidy_fix.sh

10

I'd leave the grep as it was in the test, so that it filters out RUN: lines as well, and it only filters out comments:

grep -Ev "// *[A-Z-]+:" ...

We'll have some checks for comments, so filtering out special comments can be important there.

test/clang-tidy/check_clang_tidy_output.sh

9

nit: I'd format this still a bit differently (where's my sh-format? ;), four spaces on the continuation line, and each pipe component from a new line with 2 spaces indentation:

clang-tidy --checks=${CHECK_TO_RUN} --disable-checks="" ${INPUT_FILE} \
    -- --std=c++11 \
  | FileCheck ${INPUT_FILE}

[sbenza]

Minor fix on scripts.

test/clang-tidy/check_clang_tidy_fix.sh

10

Done.

test/clang-tidy/check_clang_tidy_output.sh

9

Done.

[alexfh]

LG

[sbenza]

Submitted as rL205854

clang-tidy/misc/RedundantSmartptrGet.cpp

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
-void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {
+namespace {
+internal::Matcher<Expr> callToGet(internal::Matcher<Decl> OnClass) {
+  return memberCallExpr(
+             on(expr(anyOf(hasType(OnClass),
+                           hasType(qualType(pointsTo(decl(OnClass).bind(
+                               "ptr_to_ptr")))))).bind("smart_pointer")),
+             callee(methodDecl(hasName("get")))).bind("redundant_get");
+}
+
+void registerMatchersForGetArrowStart(MatchFinder *Finder,
+                                      MatchFinder::MatchCallback *Callback) {
   const auto QuacksLikeASmartptr = recordDecl(
+      recordDecl().bind("duck_typing"),
       has(methodDecl(hasName("operator->"),
                      returns(qualType(pointsTo(type().bind("op->Type")))))),
       has(methodDecl(hasName("operator*"),
                      returns(qualType(references(type().bind("op*Type")))))),
       has(methodDecl(hasName("get"),
                      returns(qualType(pointsTo(type().bind("getType")))))));
 
-  const auto CallToGet =
-      memberCallExpr(on(expr(hasType(recordDecl(QuacksLikeASmartptr)))
-                            .bind("smart_pointer")),
-                     callee(methodDecl(hasName("get")))).bind("redundant_get");
-
-  const auto ArrowCallToGet =
-      memberCallExpr(
-          on(expr(hasType(qualType(pointsTo(recordDecl(QuacksLikeASmartptr)))))
-                 .bind("smart_pointer")),
-          callee(methodDecl(hasName("get")))).bind("redundant_get");
-
   // Catch 'ptr.get()->Foo()'
-  Finder->addMatcher(
-      memberExpr(isArrow(), hasObjectExpression(ignoringImpCasts(CallToGet))),
-      this);
+  Finder->addMatcher(memberExpr(expr().bind("memberExpr"), isArrow(),
+                                hasObjectExpression(ignoringImpCasts(
+                                    callToGet(QuacksLikeASmartptr)))),
+                     Callback);
 
-  // Catch '*ptr.get()'
+  // Catch '*ptr.get()' or '*ptr->get()'
   Finder->addMatcher(
-      unaryOperator(hasOperatorName("*"), hasUnaryOperand(CallToGet)), this);
+      unaryOperator(hasOperatorName("*"),
+                    hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
+      Callback);
+}
 
-  // Catch '*ptr->get()'
+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")));
+
+  // Matches against nullptr.
   Finder->addMatcher(
-      unaryOperator(hasOperatorName("*"), hasUnaryOperand(ArrowCallToGet))
-          .bind("ptr_to_ptr"),
-      this);
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     hasEitherOperand(ignoringImpCasts(nullPtrLiteralExpr())),
+                     hasEitherOperand(callToGet(IsAKnownSmartptr))),
+      Callback);
+  // TODO: Catch ptr.get() == other_ptr.get()
+}
+
+
+}  // namespace
+
+void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {
+  registerMatchersForGetArrowStart(Finder, this);
+  registerMatchersForGetEquals(Finder, this);
 }
 
 namespace {
 bool allReturnTypesMatch(const MatchFinder::MatchResult &Result) {
+  if (Result.Nodes.getNodeAs<Decl>("duck_typing") == nullptr)
+    return true;
   // Verify that the types match.
   // We can't do this on the matcher because the type nodes can be different,
   // even though they represent the same type. This difference comes from how
   // the type is referenced (eg. through a typedef, a type trait, etc).
   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
 
 void RedundantSmartptrGet::check(const MatchFinder::MatchResult &Result) {
   if (!allReturnTypesMatch(Result)) return;
 
-  bool IsPtrToPtr = Result.Nodes.getNodeAs<Expr>("ptr_to_ptr") != nullptr;
+  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());
+    return;
+  }
+
   StringRef SmartptrText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(Smartptr->getSourceRange()),
       *Result.SourceManager, LangOptions());
-  // Replace *foo->get() with **foo, and foo.get() with foo.
+  // Replace foo->get() with *foo, and foo.get() with foo.
   std::string Replacement = Twine(IsPtrToPtr ? "*" : "", SmartptrText).str();
   diag(GetCall->getLocStart(), "Redundant get() call on smart pointer.")
       << FixItHint::CreateReplacement(GetCall->getSourceRange(), Replacement);
 }
 
 } // namespace tidy
 } // namespace clang

test/clang-tidy/check_clang_tidy_fix.sh

+#!/bin/sh
+#
+# Run clang-tidy in fix mode and verify the result.
+
+INPUT_FILE=$1
+CHECK_TO_RUN=$2
+TEMPORARY_FILE=$3.cpp
+
+grep -Ev "// *[A-Z-]+:" ${INPUT_FILE} > ${TEMPORARY_FILE}
+clang-tidy ${TEMPORARY_FILE} -fix --checks=${CHECK_TO_RUN} \
+    --disable-checks="" -- --std=c++11
+FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE}

test/clang-tidy/check_clang_tidy_output.sh

+#!/bin/sh
+#
+# Run clang-tidy and verify its command line output.
+
+INPUT_FILE=$1
+CHECK_TO_RUN=$2
+
+clang-tidy --checks=${CHECK_TO_RUN} --disable-checks="" ${INPUT_FILE} \
+    -- --std=c++11 \
+  | FileCheck ${INPUT_FILE}

test/clang-tidy/redundant-smartptr-get-fix.cpp

-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: clang-tidy %t.cpp -fix -checks=misc-redundant-smartptr-get --
-// RUN: FileCheck -input-file=%t.cpp %s
+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-redundant-smartptr-get %t
+
+#include <memory>
 
 struct Bar {
   void Do();
   void ConstDo() const;
 };
 struct BarPtr {
   Bar* operator->();
   Bar& operator*();
void Positive() {
 
   BarPtr* up;
   (*up->get()).Do();
   // CHECK: (**up).Do();
 
   int_ptr ip;
   int i = *ip.get();
   // CHECK: int i = *ip;
+
+  std::unique_ptr<int> uu;
+  std::shared_ptr<double> *ss;
+  bool bb = uu.get() == nullptr;
+  // CHECK: bool bb = uu == nullptr;
+  bb = nullptr != ss->get();
+  // CHECK: bb = nullptr != *ss;
 }

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

-// RUN: clang-tidy --checks=misc-redundant-smartptr-get %s -- | FileCheck %s
+// RUN: $(dirname %s)/check_clang_tidy_output.sh %s misc-redundant-smartptr-get
 
 // CHECK-NOT: warning
 
-namespace std {
-
-template <typename T>
-struct MakeRef {
-  typedef T& type;
-};
-
-template <typename T>
-struct unique_ptr {
-  T* get();
-  T* operator->();
-  // This simulates libstdc++'s implementation of unique_ptr.
-  typename MakeRef<T>::type operator*();
-};
-}  // namespace std
+#include <memory>
 
 struct int_ptr {
   int* get();
   int* operator->();
   int& operator*();
 };
 
 struct Bar {
void Positive() {
   (*up->get()).Do();
   // CHECK: :[[@LINE-1]]:5: warning: Redundant get() call on smart pointer.
   // CHECK: (*up->get()).Do();
 
   int_ptr ip;
   int i = *ip.get();
   // CHECK: :[[@LINE-1]]:12: warning: Redundant get() call on smart pointer.
   // CHECK: int i = *ip.get();
+
+  bool bb = u.get() == nullptr;
+  // CHECK: :[[@LINE-1]]:13: warning: Redundant get() call on smart pointer.
+  // CHECK: u.get() == nullptr;
+  std::shared_ptr<double> *sp;
+  bb = nullptr != sp->get();
+  // CHECK: :[[@LINE-1]]:19: warning: Redundant get() call on smart pointer.
+  // CHECK: nullptr != sp->get();
 }
 
 // CHECK-NOT: warning
 
 void Negative() {
   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;
 }