<div dir="ltr">Revision rL205913 was submitted to fix this some time ago.<br><div class="gmail_extra">Sorry for the break.</div><div class="gmail_extra">_Sam<br><br><div class="gmail_quote">On Wed, Apr 9, 2014 at 4:47 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank" class="cremed">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">This appears to have broken one of the build bots:<br>
<a href="http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/1540" target="_blank" class="cremed">http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/1540</a><br>
<br>
~Aaron<br>
<br>
On Wed, Apr 9, 2014 at 10:17 AM, Samuel Benzaquen <<a href="mailto:sbenza@google.com" class="cremed">sbenza@google.com</a>> wrote:<br>
> Author: sbenza<br>
> Date: Wed Apr 9 09:17:23 2014<br>
> New Revision: 205854<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=205854&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=205854&view=rev</a><br>
> Log:<br>
> Extend the check to detect patterns like 'ptr.get() == nullptr'<br>
><br>
> Summary:<br>
> Extend the check to detect patterns like 'ptr.get() == nullptr'<br>
> It detects == and != when any argument is a ptr.get() and the other is a<br>
> nullptr.<br>
> Only supports standard smart pointer types std::unique_ptr and std::shared_ptr.<br>
> Does not support the case 'ptr.get() == other.get()' yet.<br>
><br>
> Reviewers: djasper<br>
><br>
> CC: cfe-commits, alexfh<br>
><br>
> Differential Revision: <a href="http://llvm-reviews.chandlerc.com/D3294" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D3294</a><br>
><br>
> Added:<br>
> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh (with props)<br>
> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh (with props)<br>
> Modified:<br>
> clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp<br>
> clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp<br>
> clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp<br>
><br>
> Modified: clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp?rev=205854&r1=205853&r2=205854&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp?rev=205854&r1=205853&r2=205854&view=diff</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp (original)<br>
> +++ clang-tools-extra/trunk/clang-tidy/misc/RedundantSmartptrGet.cpp Wed Apr 9 09:17:23 2014<br>
> @@ -16,8 +16,19 @@ using namespace clang::ast_matchers;<br>
> namespace clang {<br>
> namespace tidy {<br>
><br>
> -void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {<br>
> +namespace {<br>
> +internal::Matcher<Expr> callToGet(internal::Matcher<Decl> OnClass) {<br>
> + return memberCallExpr(<br>
> + on(expr(anyOf(hasType(OnClass),<br>
> + hasType(qualType(pointsTo(decl(OnClass).bind(<br>
> + "ptr_to_ptr")))))).bind("smart_pointer")),<br>
> + callee(methodDecl(hasName("get")))).bind("redundant_get");<br>
> +}<br>
> +<br>
> +void registerMatchersForGetArrowStart(MatchFinder *Finder,<br>
> + MatchFinder::MatchCallback *Callback) {<br>
> const auto QuacksLikeASmartptr = recordDecl(<br>
> + recordDecl().bind("duck_typing"),<br>
> has(methodDecl(hasName("operator->"),<br>
> returns(qualType(pointsTo(type().bind("op->Type")))))),<br>
> has(methodDecl(hasName("operator*"),<br>
> @@ -25,35 +36,50 @@ void RedundantSmartptrGet::registerMatch<br>
> has(methodDecl(hasName("get"),<br>
> returns(qualType(pointsTo(type().bind("getType")))))));<br>
><br>
> - const auto CallToGet =<br>
> - memberCallExpr(on(expr(hasType(recordDecl(QuacksLikeASmartptr)))<br>
> - .bind("smart_pointer")),<br>
> - callee(methodDecl(hasName("get")))).bind("redundant_get");<br>
> -<br>
> - const auto ArrowCallToGet =<br>
> - memberCallExpr(<br>
> - on(expr(hasType(qualType(pointsTo(recordDecl(QuacksLikeASmartptr)))))<br>
> - .bind("smart_pointer")),<br>
> - callee(methodDecl(hasName("get")))).bind("redundant_get");<br>
> -<br>
> // Catch 'ptr.get()->Foo()'<br>
> - Finder->addMatcher(<br>
> - memberExpr(isArrow(), hasObjectExpression(ignoringImpCasts(CallToGet))),<br>
> - this);<br>
> + Finder->addMatcher(memberExpr(expr().bind("memberExpr"), isArrow(),<br>
> + hasObjectExpression(ignoringImpCasts(<br>
> + callToGet(QuacksLikeASmartptr)))),<br>
> + Callback);<br>
><br>
> - // Catch '*ptr.get()'<br>
> + // Catch '*ptr.get()' or '*ptr->get()'<br>
> Finder->addMatcher(<br>
> - unaryOperator(hasOperatorName("*"), hasUnaryOperand(CallToGet)), this);<br>
> + unaryOperator(hasOperatorName("*"),<br>
> + hasUnaryOperand(callToGet(QuacksLikeASmartptr))),<br>
> + Callback);<br>
> +}<br>
> +<br>
> +void registerMatchersForGetEquals(MatchFinder *Finder,<br>
> + MatchFinder::MatchCallback *Callback) {<br>
> + // This one is harder to do with duck typing.<br>
> + // The operator==/!= that we are looking for might be member or non-member,<br>
> + // might be on global namespace or found by ADL, might be a template, etc.<br>
> + // For now, lets keep a list of known standard types.<br>
><br>
> - // Catch '*ptr->get()'<br>
> + const auto IsAKnownSmartptr = recordDecl(<br>
> + anyOf(hasName("::std::unique_ptr"), hasName("::std::shared_ptr")));<br>
> +<br>
> + // Matches against nullptr.<br>
> Finder->addMatcher(<br>
> - unaryOperator(hasOperatorName("*"), hasUnaryOperand(ArrowCallToGet))<br>
> - .bind("ptr_to_ptr"),<br>
> - this);<br>
> + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),<br>
> + hasEitherOperand(ignoringImpCasts(nullPtrLiteralExpr())),<br>
> + hasEitherOperand(callToGet(IsAKnownSmartptr))),<br>
> + Callback);<br>
> + // TODO: Catch ptr.get() == other_ptr.get()<br>
> +}<br>
> +<br>
> +<br>
> +} // namespace<br>
> +<br>
> +void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {<br>
> + registerMatchersForGetArrowStart(Finder, this);<br>
> + registerMatchersForGetEquals(Finder, this);<br>
> }<br>
><br>
> namespace {<br>
> bool allReturnTypesMatch(const MatchFinder::MatchResult &Result) {<br>
> + if (Result.Nodes.getNodeAs<Decl>("duck_typing") == nullptr)<br>
> + return true;<br>
> // Verify that the types match.<br>
> // We can't do this on the matcher because the type nodes can be different,<br>
> // even though they represent the same type. This difference comes from how<br>
> @@ -71,14 +97,20 @@ bool allReturnTypesMatch(const MatchFind<br>
> void RedundantSmartptrGet::check(const MatchFinder::MatchResult &Result) {<br>
> if (!allReturnTypesMatch(Result)) return;<br>
><br>
> - bool IsPtrToPtr = Result.Nodes.getNodeAs<Expr>("ptr_to_ptr") != nullptr;<br>
> + bool IsPtrToPtr = Result.Nodes.getNodeAs<Decl>("ptr_to_ptr") != nullptr;<br>
> + bool IsMemberExpr = Result.Nodes.getNodeAs<Expr>("memberExpr") != nullptr;<br>
> const Expr *GetCall = Result.Nodes.getNodeAs<Expr>("redundant_get");<br>
> const Expr *Smartptr = Result.Nodes.getNodeAs<Expr>("smart_pointer");<br>
><br>
> + if (IsPtrToPtr && IsMemberExpr) {<br>
> + // Ignore this case (eg. Foo->get()->DoSomething());<br>
> + return;<br>
> + }<br>
> +<br>
> StringRef SmartptrText = Lexer::getSourceText(<br>
> CharSourceRange::getTokenRange(Smartptr->getSourceRange()),<br>
> *Result.SourceManager, LangOptions());<br>
> - // Replace *foo->get() with **foo, and foo.get() with foo.<br>
> + // Replace foo->get() with *foo, and foo.get() with foo.<br>
> std::string Replacement = Twine(IsPtrToPtr ? "*" : "", SmartptrText).str();<br>
> diag(GetCall->getLocStart(), "Redundant get() call on smart pointer.")<br>
> << FixItHint::CreateReplacement(GetCall->getSourceRange(), Replacement);<br>
><br>
> Added: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh?rev=205854&view=auto" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh?rev=205854&view=auto</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh (added)<br>
> +++ clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh Wed Apr 9 09:17:23 2014<br>
> @@ -0,0 +1,12 @@<br>
> +#!/bin/sh<br>
> +#<br>
> +# Run clang-tidy in fix mode and verify the result.<br>
> +<br>
> +INPUT_FILE=$1<br>
> +CHECK_TO_RUN=$2<br>
> +TEMPORARY_FILE=$3.cpp<br>
> +<br>
> +grep -Ev "// *[A-Z-]+:" ${INPUT_FILE} > ${TEMPORARY_FILE}<br>
> +clang-tidy ${TEMPORARY_FILE} -fix --checks=${CHECK_TO_RUN} \<br>
> + --disable-checks="" -- --std=c++11<br>
> +FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE}<br>
><br>
> Propchange: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh<br>
> ------------------------------------------------------------------------------<br>
> svn:executable = *<br>
><br>
> Added: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh?rev=205854&view=auto" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh?rev=205854&view=auto</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh (added)<br>
> +++ clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh Wed Apr 9 09:17:23 2014<br>
> @@ -0,0 +1,10 @@<br>
> +#!/bin/sh<br>
> +#<br>
> +# Run clang-tidy and verify its command line output.<br>
> +<br>
> +INPUT_FILE=$1<br>
> +CHECK_TO_RUN=$2<br>
> +<br>
> +clang-tidy --checks=${CHECK_TO_RUN} --disable-checks="" ${INPUT_FILE} \<br>
> + -- --std=c++11 \<br>
> + | FileCheck ${INPUT_FILE}<br>
><br>
> Propchange: clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_output.sh<br>
> ------------------------------------------------------------------------------<br>
> svn:executable = *<br>
><br>
> Modified: clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp<br>
> URL: <a href="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" target="_blank" class="cremed">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</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp (original)<br>
> +++ clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get-fix.cpp Wed Apr 9 09:17:23 2014<br>
> @@ -1,6 +1,6 @@<br>
> -// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp<br>
> -// RUN: clang-tidy %t.cpp -fix -checks=misc-redundant-smartptr-get --<br>
> -// RUN: FileCheck -input-file=%t.cpp %s<br>
> +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-redundant-smartptr-get %t<br>
> +<br>
> +#include <memory><br>
><br>
> struct Bar {<br>
> void Do();<br>
> @@ -39,4 +39,11 @@ void Positive() {<br>
> int_ptr ip;<br>
> int i = *ip.get();<br>
> // CHECK: int i = *ip;<br>
> +<br>
> + std::unique_ptr<int> uu;<br>
> + std::shared_ptr<double> *ss;<br>
> + bool bb = uu.get() == nullptr;<br>
> + // CHECK: bool bb = uu == nullptr;<br>
> + bb = nullptr != ss->get();<br>
> + // CHECK: bb = nullptr != *ss;<br>
> }<br>
><br>
> Modified: clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp<br>
> URL: <a href="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" target="_blank" class="cremed">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</a><br>
> ==============================================================================<br>
> --- clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp (original)<br>
> +++ clang-tools-extra/trunk/test/clang-tidy/redundant-smartptr-get.cpp Wed Apr 9 09:17:23 2014<br>
> @@ -1,22 +1,8 @@<br>
> -// RUN: clang-tidy --checks=misc-redundant-smartptr-get %s -- | FileCheck %s<br>
> +// RUN: $(dirname %s)/check_clang_tidy_output.sh %s misc-redundant-smartptr-get<br>
><br>
> // CHECK-NOT: warning<br>
><br>
> -namespace std {<br>
> -<br>
> -template <typename T><br>
> -struct MakeRef {<br>
> - typedef T& type;<br>
> -};<br>
> -<br>
> -template <typename T><br>
> -struct unique_ptr {<br>
> - T* get();<br>
> - T* operator->();<br>
> - // This simulates libstdc++'s implementation of unique_ptr.<br>
> - typename MakeRef<T>::type operator*();<br>
> -};<br>
> -} // namespace std<br>
> +#include <memory><br>
><br>
> struct int_ptr {<br>
> int* get();<br>
> @@ -65,6 +51,14 @@ void Positive() {<br>
> int i = *ip.get();<br>
> // CHECK: :[[@LINE-1]]:12: warning: Redundant get() call on smart pointer.<br>
> // CHECK: int i = *ip.get();<br>
> +<br>
> + bool bb = u.get() == nullptr;<br>
> + // CHECK: :[[@LINE-1]]:13: warning: Redundant get() call on smart pointer.<br>
> + // CHECK: u.get() == nullptr;<br>
> + std::shared_ptr<double> *sp;<br>
> + bb = nullptr != sp->get();<br>
> + // CHECK: :[[@LINE-1]]:19: warning: Redundant get() call on smart pointer.<br>
> + // CHECK: nullptr != sp->get();<br>
> }<br>
><br>
> // CHECK-NOT: warning<br>
> @@ -77,4 +71,9 @@ void Negative() {<br>
> Fail2().get()->Do();<br>
> const Bar& b = *Fail1().get();<br>
> (*Fail2().get()).Do();<br>
> +<br>
> + int_ptr ip;<br>
> + bool bb = std::unique_ptr<int>().get() == NULL;<br>
> + bb = ip.get() == nullptr;<br>
> + bb = u->get() == NULL;<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>