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