On Thu, Apr 9, 2015 at 11:12 AM, Samuel Benzaquen <[email protected]> wrote:
> > > 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. > Pretty much > With this change I disabled the wrong suggestions only. They were giving a > fix that doesn't compile. > *nod* > > >> >> >>> >>> 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
