<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 20, 2015 at 8:39 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"><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Mon, Oct 19, 2015 at 6:00 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 Mon, Oct 19, 2015 at 2:49 PM, Samuel Benzaquen via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</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: Mon Oct 19 16:49:51 2015<br>
New Revision: 250742<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=250742&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=250742&view=rev</a><br>
Log:<br>
Added check uniqueptr-delete-release to replace "delete x.release()" with "x = nullptr"<br></blockquote><div><br></div><div>Any stats on this? Have we seen many instances of "delete x.release()"<br></div></div></div></div></blockquote><div><br></div></span><div>I would say in the hundreds.</div><span class=""><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>Also, an interesting question: should the fixit be "x = nullptr" or "x.reset()" ? I remember having this discussion with at least Lang Hames & he preferred the latter, which I can see, though my initial reaction is to use the former.</div></div></div></div></blockquote><div><br></div></span><div>The opinions have been split half and half on this one, but no one feels strongly one way or another.</div><div>Given that I implemented it, I went with my preference.</div></div></div></div></blockquote><div><br></div><div>Sounds fair - thanks for the context!</div><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 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: alexfh<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D13179" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13179</a><br>
<br>
Added:<br>
clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp<br>
clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h<br>
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst<br>
clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp<br>
Modified:<br>
clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt<br>
clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp<br>
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst<br>
<br>
Modified: clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt?rev=250742&r1=250741&r2=250742&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt?rev=250742&r1=250741&r2=250742&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt (original)<br>
+++ clang-tools-extra/trunk/clang-tidy/readability/CMakeLists.txt Mon Oct 19 16:49:51 2015<br>
@@ -13,6 +13,7 @@ add_clang_library(clangTidyReadabilityMo<br>
RedundantStringCStrCheck.cpp<br>
RedundantSmartptrGetCheck.cpp<br>
SimplifyBooleanExprCheck.cpp<br>
+ UniqueptrDeleteReleaseCheck.cpp<br>
<br>
LINK_LIBS<br>
clangAST<br>
<br>
Modified: clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp?rev=250742&r1=250741&r2=250742&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp?rev=250742&r1=250741&r2=250742&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp (original)<br>
+++ clang-tools-extra/trunk/clang-tidy/readability/ReadabilityTidyModule.cpp Mon Oct 19 16:49:51 2015<br>
@@ -20,6 +20,7 @@<br>
#include "RedundantSmartptrGetCheck.h"<br>
#include "RedundantStringCStrCheck.h"<br>
#include "SimplifyBooleanExprCheck.h"<br>
+#include "UniqueptrDeleteReleaseCheck.h"<br>
<br>
namespace clang {<br>
namespace tidy {<br>
@@ -40,6 +41,8 @@ public:<br>
"readability-identifier-naming");<br>
CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(<br>
"readability-inconsistent-declaration-parameter-name");<br>
+ CheckFactories.registerCheck<UniqueptrDeleteReleaseCheck>(<br>
+ "readability-uniqueptr-delete-release");<br>
CheckFactories.registerCheck<readability::NamedParameterCheck>(<br>
"readability-named-parameter");<br>
CheckFactories.registerCheck<RedundantSmartptrGetCheck>(<br>
<br>
Added: clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp?rev=250742&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp?rev=250742&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp (added)<br>
+++ clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp Mon Oct 19 16:49:51 2015<br>
@@ -0,0 +1,69 @@<br>
+//===--- UniqueptrDeleteReleaseCheck.cpp - clang-tidy----------------------===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "UniqueptrDeleteReleaseCheck.h"<br>
+#include "clang/AST/ASTContext.h"<br>
+#include "clang/ASTMatchers/ASTMatchFinder.h"<br>
+#include "clang/Lex/Lexer.h"<br>
+<br>
+using namespace clang::ast_matchers;<br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+<br>
+void UniqueptrDeleteReleaseCheck::registerMatchers(MatchFinder *Finder) {<br>
+ auto IsSusbstituted = qualType(anyOf(<br>
+ substTemplateTypeParmType(), hasDescendant(substTemplateTypeParmType())));<br>
+<br>
+ auto UniquePtrWithDefaultDelete = classTemplateSpecializationDecl(<br>
+ hasName("std::unique_ptr"),<br>
+ hasTemplateArgument(1, refersToType(qualType(hasDeclaration(cxxRecordDecl(<br>
+ hasName("std::default_delete")))))));<br>
+<br>
+ Finder->addMatcher(<br>
+ cxxDeleteExpr(<br>
+ has(cxxMemberCallExpr(on(expr(hasType(UniquePtrWithDefaultDelete),<br>
+ unless(hasType(IsSusbstituted)))<br>
+ .bind("uptr")),<br>
+ callee(cxxMethodDecl(hasName("release"))))))<br>
+ .bind("delete"),<br>
+ this);<br>
+}<br>
+<br>
+void UniqueptrDeleteReleaseCheck::check(<br>
+ const MatchFinder::MatchResult &Result) {<br>
+ const auto *PtrExpr = Result.Nodes.getNodeAs<Expr>("uptr");<br>
+ const auto *DeleteExpr = Result.Nodes.getNodeAs<Expr>("delete");<br>
+<br>
+ if (PtrExpr->getLocStart().isMacroID())<br>
+ return;<br>
+<br>
+ // Ignore dependent types.<br>
+ // It can give us false positives, so we go with false negatives instead to<br>
+ // be safe.<br>
+ if (PtrExpr->getType()->isDependentType())<br>
+ return;<br>
+<br>
+ SourceLocation AfterPtr =<br>
+ Lexer::getLocForEndOfToken(PtrExpr->getLocEnd(), 0, *Result.SourceManager,<br>
+ Result.Context->getLangOpts());<br>
+<br>
+ diag(DeleteExpr->getLocStart(),<br>
+ "prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> "<br>
+ "objects")<br>
+ << FixItHint::CreateRemoval(CharSourceRange::getCharRange(<br>
+ DeleteExpr->getLocStart(), PtrExpr->getLocStart()))<br>
+ << FixItHint::CreateReplacement(<br>
+ CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getLocEnd()),<br>
+ " = nullptr");<br>
+}<br>
+<br>
+} // namespace tidy<br>
+} // namespace clang<br>
+<br>
<br>
Added: clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h?rev=250742&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h?rev=250742&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h (added)<br>
+++ clang-tools-extra/trunk/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h Mon Oct 19 16:49:51 2015<br>
@@ -0,0 +1,35 @@<br>
+//===--- UniqueptrDeleteReleaseCheck.h - clang-tidy--------------*- C++ -*-===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H<br>
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H<br>
+<br>
+#include "../ClangTidy.h"<br>
+<br>
+namespace clang {<br>
+namespace tidy {<br>
+<br>
+/// Flag statements of the form: delete <unique_ptr expr>.release()<br>
+/// and replace them with: <unique_ptr expr> = nullptr<br>
+///<br>
+/// For the user-facing documentation see:<br>
+/// <a href="http://clang.llvm.org/extra/clang-tidy/checks/readability-uniqueptr-delete-release.html" rel="noreferrer" target="_blank">http://clang.llvm.org/extra/clang-tidy/checks/readability-uniqueptr-delete-release.html</a><br>
+class UniqueptrDeleteReleaseCheck : public ClangTidyCheck {<br>
+public:<br>
+ UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context)<br>
+ : ClangTidyCheck(Name, Context) {}<br>
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;<br>
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;<br>
+};<br>
+<br>
+} // namespace tidy<br>
+} // namespace clang<br>
+<br>
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_UNIQUEPTR_DELETE_RELEASE_H<br>
+<br>
<br>
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=250742&r1=250741&r2=250742&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst?rev=250742&r1=250741&r2=250742&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst (original)<br>
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst Mon Oct 19 16:49:51 2015<br>
@@ -67,3 +67,4 @@ List of clang-tidy Checks<br>
readability-redundant-smartptr-get<br>
readability-redundant-string-cstr<br>
readability-simplify-boolean-expr<br>
+ readability-uniqueptr-delete-release<br>
<br>
Added: clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst?rev=250742&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst?rev=250742&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst (added)<br>
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst Mon Oct 19 16:49:51 2015<br>
@@ -0,0 +1,5 @@<br>
+readability-uniqueptr-delete-release<br>
+====================================<br>
+<br>
+Replace ``delete <unique_ptr>.release()`` with ``<unique_ptr> = nullptr``.<br>
+The latter is shorter, simpler and does not require use of raw pointer APIs.<br>
<br>
Added: clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp?rev=250742&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp?rev=250742&view=auto</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp (added)<br>
+++ clang-tools-extra/trunk/test/clang-tidy/readability-uniqueptr-delete-release.cpp Mon Oct 19 16:49:51 2015<br>
@@ -0,0 +1,71 @@<br>
+// RUN: %python %S/check_clang_tidy.py %s readability-uniqueptr-delete-release %t<br>
+<br>
+namespace std {<br>
+template <typename T><br>
+struct default_delete {};<br>
+<br>
+template <typename T, typename D = default_delete<T>><br>
+class unique_ptr {<br>
+ public:<br>
+ unique_ptr();<br>
+ ~unique_ptr();<br>
+ explicit unique_ptr(T*);<br>
+ template <typename U, typename E><br>
+ unique_ptr(unique_ptr<U, E>&&);<br>
+ T* release();<br>
+};<br>
+} // namespace std<br>
+<br>
+std::unique_ptr<int>& ReturnsAUnique();<br>
+<br>
+void Positives() {<br>
+ std::unique_ptr<int> P;<br>
+ delete P.release();<br>
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]<br>
+ // CHECK-FIXES: {{^}} P = nullptr;<br>
+<br>
+ std::unique_ptr<int> Array[20];<br>
+ delete Array[4].release();<br>
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete<br>
+ // CHECK-FIXES: {{^}} Array[4] = nullptr;<br>
+<br>
+ delete ReturnsAUnique().release();<br>
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete<br>
+ // CHECK-FIXES: {{^}} ReturnsAUnique() = nullptr;<br>
+}<br>
+<br>
+struct NotDefaultDeleter {};<br>
+<br>
+struct NotUniquePtr {<br>
+ int* release();<br>
+};<br>
+<br>
+void Negatives() {<br>
+ std::unique_ptr<int, NotDefaultDeleter> P;<br>
+ delete P.release();<br>
+<br>
+ NotUniquePtr P2;<br>
+ delete P2.release();<br>
+}<br>
+<br>
+template <typename T, typename D><br>
+void NegativeDeleterT() {<br>
+ // Ideally this would trigger a warning, but we have all dependent types<br>
+ // disabled for now.<br>
+ std::unique_ptr<T> P;<br>
+ delete P.release();<br>
+<br>
+ // We ignore this one because the deleter is a template argument.<br>
+ // Not all instantiations will use the default deleter.<br>
+ std::unique_ptr<int, D> P2;<br>
+ delete P2.release();<br>
+}<br>
+template void NegativeDeleterT<int, std::default_delete<int>>();<br>
+<br>
+// Test some macros<br>
+<br>
+#define DELETE_RELEASE(x) delete (x).release()<br>
+void NegativesWithTemplate() {<br>
+ std::unique_ptr<int> P;<br>
+ DELETE_RELEASE(P);<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>