Index: clang-tidy/misc/RedundantSmartptrGet.cpp =================================================================== --- clang-tidy/misc/RedundantSmartptrGet.cpp +++ clang-tidy/misc/RedundantSmartptrGet.cpp @@ -16,8 +16,19 @@ namespace clang { namespace tidy { -void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) { +namespace { +internal::Matcher callToGet(internal::Matcher 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*"), @@ -25,35 +36,50 @@ 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); +} + +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. - // Catch '*ptr->get()' + 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("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 @@ -71,14 +97,20 @@ void RedundantSmartptrGet::check(const MatchFinder::MatchResult &Result) { if (!allReturnTypesMatch(Result)) return; - bool IsPtrToPtr = Result.Nodes.getNodeAs("ptr_to_ptr") != nullptr; + bool IsPtrToPtr = Result.Nodes.getNodeAs("ptr_to_ptr") != nullptr; + bool IsMemberExpr = Result.Nodes.getNodeAs("memberExpr") != nullptr; const Expr *GetCall = Result.Nodes.getNodeAs("redundant_get"); const Expr *Smartptr = Result.Nodes.getNodeAs("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); Index: test/clang-tidy/check_clang_tidy_fix.sh =================================================================== --- /dev/null +++ test/clang-tidy/check_clang_tidy_fix.sh @@ -0,0 +1,12 @@ +#!/bin/sh +# +# Run clang-tidy in fix mode and verify the result. + +INPUT_FILE=$1 +CHECK_TO_RUN=$2 +TEMPORARY_FILE=$3.cpp + +grep -v CHECK ${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} Index: test/clang-tidy/check_clang_tidy_output.sh =================================================================== --- /dev/null +++ test/clang-tidy/check_clang_tidy_output.sh @@ -0,0 +1,9 @@ +#!/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} Index: test/clang-tidy/redundant-smartptr-get-fix.cpp =================================================================== --- test/clang-tidy/redundant-smartptr-get-fix.cpp +++ test/clang-tidy/redundant-smartptr-get-fix.cpp @@ -1,6 +1,6 @@ -// 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 struct Bar { void Do(); @@ -39,4 +39,11 @@ int_ptr ip; int i = *ip.get(); // CHECK: int i = *ip; + + std::unique_ptr uu; + std::shared_ptr *ss; + bool bb = uu.get() == nullptr; + // CHECK: bool bb = uu == nullptr; + bb = nullptr != ss->get(); + // CHECK: bb = nullptr != *ss; } Index: test/clang-tidy/redundant-smartptr-get.cpp =================================================================== --- test/clang-tidy/redundant-smartptr-get.cpp +++ test/clang-tidy/redundant-smartptr-get.cpp @@ -1,22 +1,8 @@ -// 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 -struct MakeRef { - typedef T& type; -}; - -template -struct unique_ptr { - T* get(); - T* operator->(); - // This simulates libstdc++'s implementation of unique_ptr. - typename MakeRef::type operator*(); -}; -} // namespace std +#include struct int_ptr { int* get(); @@ -65,6 +51,14 @@ 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 *sp; + bb = nullptr != sp->get(); + // CHECK: :[[@LINE-1]]:19: warning: Redundant get() call on smart pointer. + // CHECK: nullptr != sp->get(); } // CHECK-NOT: warning @@ -77,4 +71,9 @@ Fail2().get()->Do(); const Bar& b = *Fail1().get(); (*Fail2().get()).Do(); + + int_ptr ip; + bool bb = std::unique_ptr().get() == NULL; + bb = ip.get() == nullptr; + bb = u->get() == NULL; }