[clang-tools-extra] r205854 - Extend the check to detect patterns like 'ptr.get() == nullptr'
Aaron Ballman
aaron at aaronballman.com
Wed Apr 9 13:47:11 PDT 2014
This appears to have broken one of the build bots:
http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/1540
~Aaron
On Wed, Apr 9, 2014 at 10:17 AM, Samuel Benzaquen <sbenza at google.com> wrote:
> Author: sbenza
> Date: Wed Apr 9 09:17:23 2014
> New Revision: 205854
>
> URL: http://llvm.org/viewvc/llvm-project?rev=205854&view=rev
> Log:
> Extend the check to detect patterns like 'ptr.get() == nullptr'
>
> 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.
>
> Reviewers: djasper
>
> CC: cfe-commits, alexfh
>
> Differential Revision: http://llvm-reviews.chandlerc.com/D3294
>
> Added:
> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh (with props)
> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh (with props)
> Modified:
> clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp
> clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp
> clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp
>
> Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp?rev=205854&r1=205853&r2=205854&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp (original)
> +++ clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp Wed Apr 9 09:17:23 2014
> @@ -16,8 +16,19 @@ 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*"),
> @@ -25,35 +36,50 @@ void RedundantSmartptrGet::registerMatch
> 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<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
> @@ -71,14 +97,20 @@ bool allReturnTypesMatch(const MatchFind
> 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);
>
> Added: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh?rev=205854&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh (added)
> +++ clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh Wed Apr 9 09:17:23 2014
> @@ -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 -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}
>
> Propchange: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh
> ------------------------------------------------------------------------------
> svn:executable = *
>
> Added: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh?rev=205854&view=auto
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh (added)
> +++ clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh Wed Apr 9 09:17:23 2014
> @@ -0,0 +1,10 @@
> +#!/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}
>
> Propchange: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh
> ------------------------------------------------------------------------------
> svn:executable = *
>
> Modified: clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp?rev=205854&r1=205853&r2=205854&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp (original)
> +++ clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp Wed Apr 9 09:17:23 2014
> @@ -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 <memory>
>
> struct Bar {
> void Do();
> @@ -39,4 +39,11 @@ void Positive() {
> 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;
> }
>
> Modified: clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp?rev=205854&r1=205853&r2=205854&view=diff
> ==============================================================================
> --- clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp (original)
> +++ clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp Wed Apr 9 09:17:23 2014
> @@ -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 <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();
> @@ -65,6 +51,14 @@ void Positive() {
> 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
> @@ -77,4 +71,9 @@ void Negative() {
> 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;
> }
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list