[clang-tools-extra] r234512 - [clang-tidy] Ignore expressions with incompatible deleters.
David Blaikie
dblaikie at gmail.com
Thu Apr 9 11:15:57 PDT 2015
On Thu, Apr 9, 2015 at 11:12 AM, Samuel Benzaquen <sbenza at google.com> wrote:
>
>
> On Thu, Apr 9, 2015 at 2:08 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Thu, Apr 9, 2015 at 10:51 AM, Samuel Benzaquen <sbenza at google.com>
>> 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
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150409/02c4e08e/attachment.html>
More information about the cfe-commits
mailing list