On Thu, Apr 9, 2015 at 2:08 PM, David Blaikie <[email protected]> wrote:
> > > On Thu, Apr 9, 2015 at 10:51 AM, Samuel Benzaquen <[email protected]> > wrote: > >> Author: sbenza >> Date: Thu Apr 9 12:51:01 2015 >> New Revision: 234512 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=234512&view=rev >> Log: >> [clang-tidy] Ignore expressions with incompatible deleters. >> >> Summary: >> Do not warn on .reset(.release()) expressions if the deleters are not >> compatible. >> Using plain assignment will probably not work. >> > > Should we have a separate check for that, then? It seems error prone... > A check for imcompatible deleters + reset(release) ? It might be useful to expand the current one to give a different warning in those cases. With this change I disabled the wrong suggestions only. They were giving a fix that doesn't compile. > > >> >> Reviewers: klimek >> >> Subscribers: curdeius, cfe-commits >> >> Differential Revision: http://reviews.llvm.org/D8422 >> >> Modified: >> clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp >> >> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp >> >> Modified: >> clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp?rev=234512&r1=234511&r2=234512&view=diff >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp >> (original) >> +++ >> clang-tools-extra/trunk/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp Thu >> Apr 9 12:51:01 2015 >> @@ -22,17 +22,75 @@ void UniqueptrResetReleaseCheck::registe >> memberCallExpr( >> on(expr().bind("left")), >> callee(memberExpr().bind("reset_member")), >> callee(methodDecl(hasName("reset"), >> - ofClass(hasName("::std::unique_ptr")))), >> + >> ofClass(recordDecl(hasName("::std::unique_ptr"), >> + >> decl().bind("left_class"))))), >> has(memberCallExpr( >> on(expr().bind("right")), >> callee(memberExpr().bind("release_member")), >> - callee(methodDecl(hasName("release"), >> - >> ofClass(hasName("::std::unique_ptr"))))))) >> + callee(methodDecl( >> + hasName("release"), >> + ofClass(recordDecl(hasName("::std::unique_ptr"), >> + decl().bind("right_class")))))))) >> .bind("reset_call"), >> this); >> } >> >> +namespace { >> +const Type *getDeleterForUniquePtr(const MatchFinder::MatchResult >> &Result, >> + StringRef ID) { >> + const auto *Class = >> + Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>(ID); >> + if (!Class) >> + return nullptr; >> + auto DeleterArgument = Class->getTemplateArgs()[1]; >> + if (DeleterArgument.getKind() != TemplateArgument::Type) >> + return nullptr; >> + return DeleterArgument.getAsType().getTypePtr(); >> +} >> + >> +bool areDeletersCompatible(const MatchFinder::MatchResult &Result) { >> + const Type *LeftDeleterType = getDeleterForUniquePtr(Result, >> "left_class"); >> + const Type *RightDeleterType = getDeleterForUniquePtr(Result, >> "right_class"); >> + >> + if (LeftDeleterType->getUnqualifiedDesugaredType() == >> + RightDeleterType->getUnqualifiedDesugaredType()) { >> + // Same type. We assume they are compatible. >> + // This check handles the case where the deleters are function >> pointers. >> + return true; >> + } >> + >> + const CXXRecordDecl *LeftDeleter = >> LeftDeleterType->getAsCXXRecordDecl(); >> + const CXXRecordDecl *RightDeleter = >> RightDeleterType->getAsCXXRecordDecl(); >> + if (!LeftDeleter || !RightDeleter) >> + return false; >> + >> + if (LeftDeleter->getCanonicalDecl() == >> RightDeleter->getCanonicalDecl()) { >> + // Same class. We assume they are compatible. >> + return true; >> + } >> + >> + const auto *LeftAsTemplate = >> + dyn_cast<ClassTemplateSpecializationDecl>(LeftDeleter); >> + const auto *RightAsTemplate = >> + dyn_cast<ClassTemplateSpecializationDecl>(RightDeleter); >> + if (LeftAsTemplate && RightAsTemplate && >> + LeftAsTemplate->getSpecializedTemplate() == >> + RightAsTemplate->getSpecializedTemplate()) { >> + // They are different instantiations of the same template. We assume >> they >> + // are compatible. >> + // This handles things like std::default_delete<Base> vs. >> + // std::default_delete<Derived>. >> + return true; >> + } >> + return false; >> +} >> + >> +} // namespace >> + >> void UniqueptrResetReleaseCheck::check(const MatchFinder::MatchResult >> &Result) { >> + if (!areDeletersCompatible(Result)) >> + return; >> + >> const auto *ResetMember = >> Result.Nodes.getNodeAs<MemberExpr>("reset_member"); >> const auto *ReleaseMember = >> Result.Nodes.getNodeAs<MemberExpr>("release_member"); >> >> Modified: >> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp?rev=234512&r1=234511&r2=234512&view=diff >> >> ============================================================================== >> --- >> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp >> (original) >> +++ >> clang-tools-extra/trunk/test/clang-tidy/misc-uniqueptr-reset-release.cpp >> Thu Apr 9 12:51:01 2015 >> @@ -2,12 +2,16 @@ >> // REQUIRES: shell >> >> namespace std { >> + >> template <typename T> >> +struct default_delete {}; >> + >> +template <typename T, class Deleter = std::default_delete<T>> >> struct unique_ptr { >> - unique_ptr<T>(); >> - explicit unique_ptr<T>(T *); >> - template <typename U> >> - unique_ptr<T>(unique_ptr<U> &&); >> + unique_ptr(); >> + explicit unique_ptr(T *); >> + template <typename U, typename E> >> + unique_ptr(unique_ptr<U, E> &&); >> void reset(T *); >> T *release(); >> }; >> @@ -20,6 +24,9 @@ std::unique_ptr<Foo> Create(); >> std::unique_ptr<Foo> &Look(); >> std::unique_ptr<Foo> *Get(); >> >> +using FooFunc = void (*)(Foo *); >> +using BarFunc = void (*)(Bar *); >> + >> void f() { >> std::unique_ptr<Foo> a, b; >> std::unique_ptr<Bar> c; >> @@ -44,5 +51,20 @@ void f() { >> Get()->reset(Get()->release()); >> // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 = >> std::move(ptr2) >> // CHECK-FIXES: *Get() = std::move(*Get()); >> + >> + std::unique_ptr<Bar, FooFunc> func_a, func_b; >> + func_a.reset(func_b.release()); >> + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: prefer ptr1 = >> std::move(ptr2) >> + // CHECK-FIXES: func_a = std::move(func_b); >> } >> >> +void negatives() { >> + std::unique_ptr<Foo> src; >> + struct OtherDeleter {}; >> + std::unique_ptr<Foo, OtherDeleter> dest; >> + dest.reset(src.release()); >> + >> + std::unique_ptr<Bar, FooFunc> func_a; >> + std::unique_ptr<Bar, BarFunc> func_b; >> + func_a.reset(func_b.release()); >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
