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