[clang-tools-extra] r234512 - [clang-tidy] Ignore expressions with incompatible deleters.
Samuel Benzaquen
sbenza at google.com
Thu Apr 9 11:12:04 PDT 2015
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.
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
>> 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/73ade8a5/attachment.html>
More information about the cfe-commits
mailing list